Repository navigation
Conversation
The Starlight docs migration (#1811) introduced root-relative links that lychee cannot resolve pre-build. Move book/src/content/docs out of the pre-build lychee target and add a post-build check against book/dist with --root-dir. Five turmoil tests broke after the terminal-hold shutdown model (#1808) changed how worker panics are handled: the pipeline now stops ingestion on held-ticket acks instead of recycling workers. Update test assertions to match the new contract, fix a checkpoint flush timing issue by using a delayed sink, and correct a validator assertion string. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThe workflow's lychee link-check step was narrowed to target 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⚔️ Resolve merge conflicts
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review This PR updates CI workflows and test assertions to fix CI failures, with no production code changes. However, multiple unresolved review comments raise valid concerns that the test assertions may be too weak to properly validate terminal-hold behavior and could miss regressions, and a CI path may be incorrect. No code changes detected at You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Pull request overview
This PR fixes CI failures in two areas: documentation link checking (Lychee + Starlight build order) and Rust Turmoil simulation tests updated for the post-#1808 terminal-hold shutdown model.
Changes:
- Adjust docs workflow to run Lychee against markdown pre-build and against
book/distpost-build with--root-dir. - Update Turmoil simulation tests to reflect terminal-hold behavior (panic/hold triggers immediate shutdown; no worker recycling).
- Stabilize a checkpoint crash timing test by introducing deterministic sink delays so simulated time advances enough for flush intervals.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
crates/logfwd/tests/turmoil_sim/trace_validation.rs |
Updates trace phase emission and validator expectation to match terminal-hold shutdown semantics. |
crates/logfwd/tests/turmoil_sim/fault_scenario_sim.rs |
Makes post_durability_flush_failure timing deterministic via delayed sink script and adjusted crash/shutdown timing. |
crates/logfwd/tests/turmoil_sim/bug_hunt.rs |
Revises panic-related invariants/assertions for terminal-hold semantics (shutdown stops ingestion). |
.github/workflows/docs.yml |
Splits Lychee into pre-build markdown check and post-build book/dist check with --root-dir. |
| let shutdown = CancellationToken::new(); | ||
| let sd = shutdown.clone(); | ||
| let phase_trace = trace.clone(); | ||
| tokio::spawn(async move { | ||
| tokio::time::sleep(Duration::from_secs(30)).await; | ||
| phase_trace.record(TraceEvent::Phase { | ||
| phase: TracePhase::Draining, | ||
| }); | ||
| sd.cancel(); |
| #[test] | ||
| fn panic_first_batch_recycles_worker_without_checkpoint_advance() { | ||
| let mut sim = super::build_sim(90, 1); |
| if durable.is_some() { | ||
| ckpt_handle.assert_durable_not_ahead_of_updates(1); | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab54be2111
ℹ️ 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".
| assert!( | ||
| delivered > 0, | ||
| "expected worker recycle to allow post-panic delivery; delivered={delivered}" | ||
| calls >= 1, | ||
| "expected at least the panic send call; calls={calls}" | ||
| ); |
There was a problem hiding this comment.
The comments for panic_first_batch_recycles_worker_without_checkpoint_advance now state that terminal-hold should stop ingestion immediately after the panic, but this assertion only requires calls >= 1 and never checks that later batches were not delivered. If worker recycling accidentally returns, this test can still pass because the held first ticket keeps checkpoint updates at zero. Add a negative assertion (for example delivered == 0 and/or an upper bound on calls) so the test actually detects post-panic recovery regressions.
Useful? React with 👍 / 👎.
| assert!( | ||
| calls >= 3, | ||
| "expected at least success + panic + recovery calls; calls={calls}" | ||
| ); | ||
| assert!( | ||
| durable.is_some(), | ||
| "first successful batch should commit some checkpoint progress" | ||
| ); | ||
| assert!( | ||
| durable.unwrap_or_default() < input_total_bytes, | ||
| "checkpoint must stay behind full input after unresolved panic gap; durable={durable:?} input_total_bytes={input_total_bytes}" | ||
| ); | ||
| calls >= 2, | ||
| "expected at least success + panic calls; calls={calls}" | ||
| ); |
There was a problem hiding this comment.
In panic_after_initial_success_does_not_advance_checkpoint_past_gap, the updated expectation says there should be no recovery batches after the panic, but calls >= 2 allows any number of additional send attempts. That means a regression back to worker recycling could slip through while still satisfying the checkpoint assertions. Tighten this check with an upper bound tied to the intended terminal-hold behavior.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/logfwd/tests/turmoil_sim/bug_hunt.rs`:
- Around line 452-453: Rename the test identifier and any in-code references
that still include "recycles_worker" to reflect the new semantics (terminal
shutdown with no worker recycling); specifically update the test function name
that contains "recycles_worker" and the surrounding comment/header "Test 6:
Panic on first batch triggers terminal shutdown, checkpoint held" so they match
(e.g., change the function name to something like
"...terminal_shutdown_no_recycle" and adjust any asserts/messages that reference
the old name).
- Around line 513-515: Replace the weak lower-bound assertions on the "calls"
counter with strict equality checks so the test fails if any post-panic sends
occur: locate the assertions referencing the variable calls that currently use
">= 1" (and the similar ">= 2" at the later site) and change them to assert that
calls == 1 (or == 2 where appropriate) with an updated message (e.g. "expected
exactly the panic send call; calls={calls}") so the test enforces no activity
after the terminal hold; apply the same change to the similar assertion block
around lines 605–607.
In `@crates/logfwd/tests/turmoil_sim/trace_validation.rs`:
- Around line 229-234: The test is synthesizing TracePhase::Draining which masks
regressions; remove the explicit call trace.record(TraceEvent::Phase { phase:
TracePhase::Draining }) so the test relies on run_async to emit phases itself
(adjust any expectations/assertions that assumed a synthetic Draining event
accordingly). Locate the synthetic call in the test (the trace.record(...) line)
and delete it, then run the test and update expected event sequences to only
include runtime-originated phases emitted by run_async.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59c89320-967b-4f56-a399-2b86f05a3d6b
📒 Files selected for processing (4)
.github/workflows/docs.ymlcrates/logfwd/tests/turmoil_sim/bug_hunt.rscrates/logfwd/tests/turmoil_sim/fault_scenario_sim.rscrates/logfwd/tests/turmoil_sim/trace_validation.rs
| // Test 6: Panic on first batch triggers terminal shutdown, checkpoint held | ||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Rename Test 6 to match updated semantics (no worker recycle).
The heading now states terminal shutdown with no recycling, but the test name still says recycles_worker, which is misleading.
Suggested rename
-fn panic_first_batch_recycles_worker_without_checkpoint_advance() {
+fn panic_first_batch_triggers_terminal_shutdown_without_checkpoint_advance() {Also applies to: 469-471
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd/tests/turmoil_sim/bug_hunt.rs` around lines 452 - 453, Rename
the test identifier and any in-code references that still include
"recycles_worker" to reflect the new semantics (terminal shutdown with no worker
recycling); specifically update the test function name that contains
"recycles_worker" and the surrounding comment/header "Test 6: Panic on first
batch triggers terminal shutdown, checkpoint held" so they match (e.g., change
the function name to something like "...terminal_shutdown_no_recycle" and adjust
any asserts/messages that reference the old name).
| calls >= 1, | ||
| "expected at least the panic send call; calls={calls}" | ||
| ); |
There was a problem hiding this comment.
Call-count assertions are too weak to prove terminal-hold behavior.
Lower bounds (>= 1, >= 2) still pass if worker recycling resumes and extra post-panic sends occur. These tests should enforce no post-panic activity.
Suggested fix
- assert!(
- calls >= 1,
- "expected at least the panic send call; calls={calls}"
- );
+ assert_eq!(
+ calls, 1,
+ "terminal-hold should stop after first panic call; calls={calls}"
+ );- assert!(
- calls >= 2,
- "expected at least success + panic calls; calls={calls}"
- );
+ assert_eq!(
+ calls, 2,
+ "terminal-hold should stop after success + panic only; calls={calls}"
+ );Also applies to: 605-607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd/tests/turmoil_sim/bug_hunt.rs` around lines 513 - 515, Replace
the weak lower-bound assertions on the "calls" counter with strict equality
checks so the test fails if any post-panic sends occur: locate the assertions
referencing the variable calls that currently use ">= 1" (and the similar ">= 2"
at the later site) and change them to assert that calls == 1 (or == 2 where
appropriate) with an updated message (e.g. "expected exactly the panic send
call; calls={calls}") so the test enforces no activity after the terminal hold;
apply the same change to the similar assertion block around lines 605–607.
| // Under the terminal-hold model, the pipeline shuts down when the | ||
| // panic ack is received. Emit Draining + Stopped in sequence to | ||
| // reflect the actual drain path that run_async executes internally. | ||
| trace.record(TraceEvent::Phase { | ||
| phase: TracePhase::Draining, | ||
| }); |
There was a problem hiding this comment.
Do not synthesize Draining in this contract test.
Recording TracePhase::Draining after run_async makes the test self-fulfilling and can hide regressions where runtime phase emission breaks. This suite is meant to validate runtime-originated contracts, not inject them.
Suggested fix
- // Under the terminal-hold model, the pipeline shuts down when the
- // panic ack is received. Emit Draining + Stopped in sequence to
- // reflect the actual drain path that run_async executes internally.
- trace.record(TraceEvent::Phase {
- phase: TracePhase::Draining,
- });
trace.record(TraceEvent::Phase {
phase: TracePhase::Stopped,
});📝 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.
| // Under the terminal-hold model, the pipeline shuts down when the | |
| // panic ack is received. Emit Draining + Stopped in sequence to | |
| // reflect the actual drain path that run_async executes internally. | |
| trace.record(TraceEvent::Phase { | |
| phase: TracePhase::Draining, | |
| }); | |
| trace.record(TraceEvent::Phase { | |
| phase: TracePhase::Stopped, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd/tests/turmoil_sim/trace_validation.rs` around lines 229 - 234,
The test is synthesizing TracePhase::Draining which masks regressions; remove
the explicit call trace.record(TraceEvent::Phase { phase: TracePhase::Draining
}) so the test relies on run_async to emit phases itself (adjust any
expectations/assertions that assumed a synthetic Draining event accordingly).
Locate the synthetic call in the test (the trace.record(...) line) and delete
it, then run the test and update expected event sequences to only include
runtime-originated phases emitted by run_async.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dismissing prior approval to re-evaluate 8e2e4c8
| --root-dir book/dist/memagent | ||
| book/dist/memagent |
There was a problem hiding this comment.
🟡 Medium workflows/docs.yml:94
The post-build lychee step targets book/dist/memagent, but Astro builds to book/dist/ directly — the base: '/memagent' config only sets URL prefixes in the generated HTML, not the physical directory structure. Since book/dist/memagent doesn't exist, lychee will error or silently pass without checking any links. Consider removing the /memagent suffix so the step checks book/dist/.
| --root-dir book/dist/memagent | |
| book/dist/memagent | |
| --root-dir book/dist |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file .github/workflows/docs.yml around lines 94-95:
The post-build lychee step targets `book/dist/memagent`, but Astro builds to `book/dist/` directly — the `base: '/memagent'` config only sets URL prefixes in the generated HTML, not the physical directory structure. Since `book/dist/memagent` doesn't exist, lychee will error or silently pass without checking any links. Consider removing the `/memagent` suffix so the step checks `book/dist/`.
7cc6723 to
2d4c93e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/docs.yml:
- Line 128: The file contains an extra trailing blank line at EOF causing a YAML
lint empty-lines error; open .github/workflows/docs.yml and remove the extra
blank line so the file ends with exactly one newline character (ensure only a
single newline at EOF).
- Around line 94-95: The lychee invocation currently points to the nested output
path "book/dist/memagent" (both the --root-dir argument and the positional
path), but Astro outputs to "book/dist" and additional docs/benchmarks are added
under that root; update both occurrences to use "book/dist" so lychee scans the
full generated site (replace "book/dist/memagent" with "book/dist" in the
workflow where the --root-dir flag and the path argument are passed).
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad87ed2c-a464-41fc-94c1-efda13e04071
📒 Files selected for processing (1)
.github/workflows/docs.yml
| --root-dir book/dist/memagent | ||
| book/dist/memagent |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify Astro config does not override output directory
rg -n "^\s*(base|outDir)\s*:" book/astro.config.mjs
# Verify workflow currently points lychee at dist/memagent
rg -n "root-dir|book/dist/memagent|book/dist" .github/workflows/docs.ymlRepository: strawgate/memagent
Length of output: 352
🏁 Script executed:
# Get the full astro.config.mjs to see complete configuration
cat book/astro.config.mjsRepository: strawgate/memagent
Length of output: 2519
🏁 Script executed:
# Get the full docs.yml workflow to see the context around the build and lychee commands
sed -n '80,120p' .github/workflows/docs.ymlRepository: strawgate/memagent
Length of output: 1177
Use book/dist for post-build lychee root/target.
Lines 94-95 point lychee to book/dist/memagent, but Astro's base: '/memagent' is only a URL prefix and doesn't change the physical output directory. Astro defaults to book/dist, and the workflow adds API docs and benchmarks as subdirectories there. Lychee should check book/dist to include all content.
Proposed fix
- --root-dir book/dist/memagent
- book/dist/memagent
+ --root-dir book/dist
+ book/dist🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/docs.yml around lines 94 - 95, The lychee invocation
currently points to the nested output path "book/dist/memagent" (both the
--root-dir argument and the positional path), but Astro outputs to "book/dist"
and additional docs/benchmarks are added under that root; update both
occurrences to use "book/dist" so lychee scans the full generated site (replace
"book/dist/memagent" with "book/dist" in the workflow where the --root-dir flag
and the path argument are passed).
| - name: Deploy to GitHub Pages | ||
| id: deploy | ||
| uses: actions/deploy-pages@v5 | ||
|
|
There was a problem hiding this comment.
Remove the extra trailing blank line.
YAMLlint reports empty-lines at Line 128; keep a single newline at EOF.
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 128-128: too many blank lines (1 > 0)
(empty-lines)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/docs.yml at line 128, The file contains an extra trailing
blank line at EOF causing a YAML lint empty-lines error; open
.github/workflows/docs.yml and remove the extra blank line so the file ends with
exactly one newline character (ensure only a single newline at EOF).
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.github/workflows/docs.yml (2)
128-128:⚠️ Potential issue | 🟡 MinorRemove the extra trailing blank line at EOF.
Line 128 still triggers YAMLlint
empty-lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docs.yml at line 128, Remove the extra trailing blank line at the end of the docs workflow file so the file ends on the last YAML content line; open .github/workflows/docs.yml and delete the empty line at EOF to satisfy YAMLlint (remove the final blank line after the last job/step definition).
84-95:⚠️ Potential issue | 🔴 CriticalFix post-build lychee root/target path to
book/dist(notbook/dist/memagent).At Line 94 and Line 95, the workflow points lychee to
book/dist/memagent, but the docs build output root isbook/dist(withbasebeing URL prefixing). This can miss content and/or fail path resolution.Proposed fix
- --root-dir book/dist/memagent - book/dist/memagent + --root-dir book/dist + book/dist#!/bin/bash set -euo pipefail # Verify Astro config uses base but does not override outDir. rg -n '^\s*(base|outDir)\s*:' book/astro.config.mjs # Verify current docs workflow lychee root-dir/target values. rg -n 'root-dir|book/dist/memagent|book/dist' .github/workflows/docs.yml # Optional focused context sed -n '84,100p' .github/workflows/docs.yml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/docs.yml around lines 84 - 95, The lychee link-check step named "Link check (Starlight built output)" is pointed at book/dist/memagent for both --root-dir and the target path which is incorrect; update the lycheeverse/lychee-action args to use --root-dir book/dist and the target path book/dist (replace both occurrences of book/dist/memagent) so lychee checks the actual built docs output directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/docs.yml:
- Line 128: Remove the extra trailing blank line at the end of the docs workflow
file so the file ends on the last YAML content line; open
.github/workflows/docs.yml and delete the empty line at EOF to satisfy YAMLlint
(remove the final blank line after the last job/step definition).
- Around line 84-95: The lychee link-check step named "Link check (Starlight
built output)" is pointed at book/dist/memagent for both --root-dir and the
target path which is incorrect; update the lycheeverse/lychee-action args to use
--root-dir book/dist and the target path book/dist (replace both occurrences of
book/dist/memagent) so lychee checks the actual built docs output directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 727a0163-4b25-4cfb-a7e5-5d367a5842c6
📒 Files selected for processing (1)
.github/workflows/docs.yml
|
Recreating to fix CI trigger issue |
Summary
book/src/content/docsout of the pre-build lychee target list in.github/workflows/docs.yml(root-relative Starlight links like/configuration/reference/fail before the Astro build)book/distwith--root-dir book/distso Starlight routes are validated after renderingpost_durability_flush_failuretest timing by using a delayed sink to ensure checkpoint flush intervals elapse before the crash is armedtrace_validator_rejects_sink_activity_after_stoppedto match actual error formatTest plan
cargo test -p logfwd --features turmoil --test turmoil_sim)cargo clippy --workspace -- -D warningscleancargo fmt --checkclean🤖 Generated with Claude Code
Note
Fix lychee link check and turmoil simulation CI failures
README.md,dev-docs, anddocs, and a new step for the built Starlight output underbook/dist/memagent.bug_hunt.rsandfault_scenario_sim.rsto match the terminal-hold shutdown model, where a sink panic causes immediate shutdown without worker recycling or post-panic deliveries.trace_validation.rstests to emitDrainingandStoppedphases in the correct order afterrun_asyncreturns, and updates an expected error substring to match the validator's current phrasing.Macroscope summarized dc47bac.