Add generic operation lifecycle for non-HTTP flows - #15
Merged
Conversation
… Added event access tests, improved level validation functions, and updated operation end API. Updated .gitignore to include development artifacts.
- Introduced NormalizeConfig function to clamp and normalize configuration values, including sampling rates and operation policies. - Enhanced error handling in event logging to provide structured error metadata. - Updated tests to validate new context behavior and error normalization. - Refactored event and operation handling to improve consistency and clarity.
- Added message and level fields to the snapshot structure for improved event context. - Updated snapshot method to include new fields for better event handling. - Refactored Commit function to utilize the new snapshot structure for message retrieval. - Introduced new operation lifecycle management files to support non-HTTP flows and improve operation handling.
- Added 'go fix' command to the tidy process in Justfile for improved code formatting. - Introduced a new skill document for 'desloppify', detailing its usage for codebase health scanning and technical debt management. - Removed unused methods and parameters from lifecycle management functions to streamline event handling. - Updated middleware implementations across various frameworks to eliminate redundant HTTP method and path fields.
…ion policies - Introduced unit tests for NormalizeConfig to validate clamping and filtering of configuration values. - Added tests to ensure event fields return shallow copies, preventing unintended mutations. - Implemented tests for operation policies to verify normalization and sampling behavior. - Enhanced structured error handling tests to preserve context and ensure accurate error reporting.
- Updated TestNewContextNilFallsBackToBackground to initialize context with a nil parent explicitly. - Modified TestBeginOperationAddsEnvelopeFields to use a nil parent context when beginning an operation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR introduces a complete operation lifecycle API for non-HTTP background-job operations (alongside existing HTTP request tracking), updates logging adapters to support deterministic field ordering and operation-aware defaults, refactors integration middleware to use the new finalization flow, adds a worker background-job module with metadata/completion lifecycle, and extends release automation with lockstep module syncing and publish/index refresh scripts. ChangesOperation Lifecycle and Core Infrastructure
Default Messages and Adapter Alignment
Integration Middleware and Worker Module
Examples, Tests, and Release Tooling
Sequence DiagramsequenceDiagram
participant Handler
participant workerhc.Start
participant hc.BeginOperation
participant hc.Add
participant op.End
participant hc.FinishOperation
participant cfg.Sink
Handler->>workerhc.Start: Start(ctx, meta)
workerhc.Start->>hc.BeginOperation: BeginOperation(ctx, OperationStart{Domain:Job})
hc.BeginOperation->>hc.BeginOperation: NewContext + apply op fields
workerhc.Start->>hc.Add: Add(op.Context(), "job.*", values)
Handler->>op.End: defer op.End(cfg, &err)
op.End->>hc.FinishOperation: FinishOperation(cfg, OperationFinish{...})
hc.FinishOperation->>hc.FinishOperation: Resolve outcome, annotate, build SampleInput
hc.FinishOperation->>cfg.Sink: Write with operation_completed message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
.agents/skills/desloppify/SKILL.md (1)
13-13: Consider Americanizing spelling for consistency."Maximise" uses British spelling. While not incorrect, American spelling ("Maximize") is more common in technical documentation and may improve consistency across global development teams.
🔤 Optional spelling adjustment
-Maximise the **strict score** honestly. Your main cycle: **scan → plan → execute → rescan**. Follow the scan output's **INSTRUCTIONS FOR AGENTS** — don't substitute your own analysis. +Maximize the **strict score** honestly. Your main cycle: **scan → next → plan → execute → rescan**. Follow the scan output's **INSTRUCTIONS FOR AGENTS** — don't substitute your own analysis.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/desloppify/SKILL.md at line 13, Change the British spelling "Maximise" to the American spelling "Maximize" in the SKILL.md content for consistency (search for the token "Maximise" and replace it with "Maximize"); ensure any occurrences in headings, examples, or descriptive text related to the "Desloppify" skill are updated and run a quick spellcheck to catch any additional British variants.metadata.go (1)
26-31: Consider handling nil recovered values.
structuredPanicFielddoesn't check for nil, which could produce"type": "<nil>", "value": "<nil>". While panics with nil values are rare, adding a nil check would provide consistency withstructuredErrorField.💡 Optional: Add nil check
func structuredPanicField(recovered any) map[string]any { + if recovered == nil { + return nil + } return map[string]any{ "type": fmt.Sprintf("%T", recovered), "value": fmt.Sprint(recovered), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metadata.go` around lines 26 - 31, structuredPanicField should handle a nil recovered value like structuredErrorField does; add a nil check at the start of structuredPanicField (function name: structuredPanicField) and when recovered == nil return a map with "type" set to "nil" and "value" set to nil (or the same representation used in structuredErrorField) otherwise proceed to build the existing map using fmt.Sprintf and fmt.Sprint.cmd/examples/sampling-custom/main_test.go (1)
32-36: Usecfg.Samplerin assertions to validate wiring, not only local function behavior.Right now the test exercises
customSamplerdirectly and then discardscfg. Calling throughcfg.Samplermakes the test intent tighter.Proposed refactor
for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - result := customSampler(tc.input) + if cfg.Sampler == nil { + t.Fatal("expected sampler to be configured") + } + result := cfg.Sampler(tc.input) if result != tc.expected { t.Errorf("customSampler() = %v, want %v", result, tc.expected) } }) } - - _ = cfg }Also applies to: 75-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/examples/sampling-custom/main_test.go` around lines 32 - 36, The test currently calls the local customSampler directly and ignores the assembled config; update the assertions to invoke the sampler via cfg.Sampler (where cfg is the hc.Config instance constructed with Sink, Sampler: customSampler, Message) so you validate the wiring through hc.Config rather than only the standalone function; replace direct calls to customSampler with cfg.Sampler in both the first occurrence and the later similar block (the other test around the same file), and keep existing expectations/assertions identical (including any returned values or error checks) to ensure behavior is unchanged while verifying the config wiring.event_access_test.go (1)
92-113: Document shallow copy semantics in public API.The test correctly validates that
EventFieldsreturns a shallow copy. However, this behavior (nested maps sharing references) could surprise callers who modify returned fields. Consider adding a doc comment onEventFieldsnoting this behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@event_access_test.go` around lines 92 - 113, Add a doc comment to the public function EventFields explaining it returns a shallow copy of the event's fields: mutations to top-level keys on the returned map do not affect the original event, but nested reference types (e.g., map[string]any, slices, pointers) are shared by reference and can be mutated through the returned map; update the comment near the EventFields declaration to state this behavior and its implications for callers.cmd/examples/router-gin/main_test.go (1)
20-21: Consider using a buffer to capture and verify log output.Unlike the
adapter-slogtest which usesbytes.Bufferto capture log output for assertions, this test writes toos.Stdout. This means the test can only verify HTTP status codes but not the actual logging behavior of the middleware.♻️ Use buffer for testable output
+ var buf bytes.Buffer - logger := slog.New(slog.NewJSONHandler(os.Stdout, nil)) + logger := slog.New(slog.NewJSONHandler(&buf, nil)) sink := sloghc.New(logger)Then add assertions in subtests:
buf.Reset() // at start of each subtest // ... after ServeHTTP ... output := buf.String() if !strings.Contains(output, "router") { t.Error("expected log output to contain 'router'") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/examples/router-gin/main_test.go` around lines 20 - 21, The test currently creates the logger with slog.NewJSONHandler(os.Stdout, nil) and cannot assert log output; replace the stdout sink with a bytes.Buffer (create buf := &bytes.Buffer{} and pass it to slog.NewJSONHandler) and construct the sloghc logger from that buffer (sloghc.New(logger)), then in each subtest call buf.Reset() before exercising the middleware and after ServeHTTP inspect buf.String() and add assertions (e.g., check strings.Contains(output, "router") or other expected fields) to verify middleware logging behavior; update references to logger and sloghc.New in main_test.go accordingly.operation_policy.go (1)
8-9: Clarify the distinction betweenCodeandStatusCode.The condition checks both
in.Code >= 500andin.StatusCode >= 500. If these represent the same concept (HTTP status), this is redundant. If they differ (e.g.,Codefor operation-specific codes vsStatusCodefor HTTP), consider adding a comment explaining the distinction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operation_policy.go` around lines 8 - 9, The condition uses both in.Code and in.StatusCode alongside in.HasError and in.Outcome != OutcomeSuccess, which is ambiguous; either consolidate to a single status field or add a short clarifying comment and/or rename fields to show their distinct meanings. Update the boolean check around in.HasError/in.Code/in.StatusCode/in.Outcome (the expression containing in.Code >= 500 and in.StatusCode >= 500) to remove redundancy if they represent the same HTTP status, or keep both but add a comment above the condition explaining that Code is an operation-specific code while StatusCode is the HTTP response code (or vice versa), and adjust any names or logic in the function that references these fields to match the clarified semantics.event_test.go (1)
116-130: Clarify the expected normalization behavior for framework-style errors.The test expects
errField["message"] == "boom", butframeworkStyleError.Error()returns"code=500, message=boom". This impliessetErrorhas special handling to extract theMessagefield from structured error types rather than usingError().If this is intentional, consider adding a brief comment in the test explaining this normalization behavior to help future maintainers understand the expectation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@event_test.go` around lines 116 - 130, The test TestSetErrorNormalizesFrameworkStyleErrors relies on setError extracting the Message field from frameworkStyleError (rather than using frameworkStyleError.Error() which returns "code=500, message=boom"); add a short clarifying comment above the test (or inside it) stating that setError intentionally normalizes structured frameworkStyleError values by reading the Message field and setting the structured "error" map["message"] to that value so future maintainers understand why the raw Error() string is not used; reference the setError method and the frameworkStyleError type in the comment for clarity.cmd/examples/adapter-slog/main_test.go (1)
77-86: Consider asserting on log output for debug and failure cases.The debug subtest resets the buffer and sets
?debug=1, but doesn't verify that the debug level was actually applied or logged. Similarly, the failure subtest doesn't verify that the error/failure was captured in the logs. This weakens the test's ability to catch regressions in level handling or error logging.💡 Suggested assertions
t.Run("request with debug flag", func(t *testing.T) { buf.Reset() req := httptest.NewRequest("GET", "/users/789?debug=1", nil) rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) if rec.Code != http.StatusOK { t.Errorf("expected status 200, got %d", rec.Code) } + output := buf.String() + if !strings.Contains(output, "requested_level") { + t.Error("expected log output to contain 'requested_level'") + } }) t.Run("request with failure", func(t *testing.T) { buf.Reset() req := httptest.NewRequest("GET", "/users/999?fail=1", nil) rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) if rec.Code != http.StatusInternalServerError { t.Errorf("expected status 500, got %d", rec.Code) } + output := buf.String() + if !strings.Contains(output, "request handled") { + t.Error("expected log output to contain 'request handled'") + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/examples/adapter-slog/main_test.go` around lines 77 - 86, The "request with debug flag" subtest resets buf and issues GET /users/789?debug=1 but never asserts that debug logging occurred; update that subtest to assert buf.String() contains the expected debug-level marker (e.g. "level=debug" or the specific debug message produced by handler) after handler.ServeHTTP(rec, req), and similarly update the failure subtest to reset buf, invoke the failing request, then assert buf.String() contains the expected error/failure log (e.g. the error text or "level=error"). Use the existing variables buf and handler to locate the code and keep assertions resilient by matching substrings rather than full timestamps.integration/worker/worker_test.go (1)
82-103: Add guard for event slice access in panic subtest.Line 96 accesses
sink.Events()[0]without checking if the slice is non-empty. If the panic handling is broken and no event is written, this will panic with an index-out-of-bounds error, making debugging harder.💡 Add defensive check
}() - ev := sink.Events()[0] + events := sink.Events() + if len(events) == 0 { + t.Fatal("expected panic event to be written") + } + ev := events[0] if ev.Fields["op.outcome"] != string(hc.OutcomePanic) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/worker/worker_test.go` around lines 82 - 103, The panic subtest currently assumes sink.Events() has at least one element and directly uses sink.Events()[0]; add a guard to check the events slice length (from sink.Events()) before indexing so the test fails with a clear message if no events were recorded. Locate the subtest using Start, JobMeta, hc.NewTestSink, op.End and hc.Config, call events := sink.Events(), assert len(events) > 0 (or t.Fatalf with a helpful message) before accessing events[0], then continue the existing assertions against events[0] and hc.OutcomePanic and the "panic" field.integration/worker/worker.go (1)
34-45: Consider simplifyingaddJobFieldsby using variadichc.Adddirectly.The slice construction with type assertion on line 45 (
kv[0].(string)) works but adds unnecessary complexity. The patternhc.Add(ctx, kv[0].(string), kv[1], kv[2:]...)unpacks a slice that was just built with string literals.♻️ Simplified approach
func addJobFields(ctx context.Context, meta JobMeta) { - kv := []any{ - "job.name", meta.Name, - "job.id", meta.ID, - "job.queue", meta.Queue, - "job.attempt", meta.Attempt, - "job.max_attempts", meta.MaxAttempts, - } - if !meta.ScheduledAt.IsZero() { - kv = append(kv, "job.scheduled_at", meta.ScheduledAt.UTC()) - } - hc.Add(ctx, kv[0].(string), kv[1], kv[2:]...) + hc.Add(ctx, + "job.name", meta.Name, + "job.id", meta.ID, + "job.queue", meta.Queue, + "job.attempt", meta.Attempt, + "job.max_attempts", meta.MaxAttempts, + ) + if !meta.ScheduledAt.IsZero() { + hc.Add(ctx, "job.scheduled_at", meta.ScheduledAt.UTC()) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/worker/worker.go` around lines 34 - 45, The current addJobFields builds a kv slice then uses kv[0].(string) which is unnecessary; instead call hc.Add directly with the key/value pairs (e.g. hc.Add(ctx, "job.name", meta.Name, "job.id", meta.ID, "job.queue", meta.Queue, "job.attempt", meta.Attempt, "job.max_attempts", meta.MaxAttempts)), and handle the optional scheduled time by calling hc.Add(ctx, "job.scheduled_at", meta.ScheduledAt.UTC()) only when meta.ScheduledAt.IsZero() is false; update the addJobFields function to remove the kv slice and type assertion and use these direct variadic hc.Add calls.operation_lifecycle.go (1)
91-100: Use one source of truth for the event during finalization.
finishOperationreappliesop.*fields viactx, but completion/failure fields and the final write all target the explicitevent. If a caller ever passes a mismatchedCtx/Eventpair, one event gets mutated and a different one gets emitted. Reapplying start fields directly toevent(or failing fast when the pair does not match) would make this helper much safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operation_lifecycle.go` around lines 91 - 100, finishOperation currently reapplies OperationStart fields into ctx via applyOperationStartFields(ctx, start) but then mutates and emits the separate event variable, which can cause mismatched/incorrect output if ctx and event differ; update finishOperation so the operation start fields are applied directly to the event before computing outcome and persisting (i.e., replace applyOperationStartFields(ctx, start) with applying the same start-to-event mapping to the event object or add/use an applyOperationStartFieldsToEvent(event, start) helper), or alternatively validate that ctx and event reference the same underlying object and fail fast; ensure all subsequent writes (resolveOutcome, final sink write) use that single event instance (references: finishOperation, applyOperationStartFields, event, OperationStart, policyForDomain, resolveOutcome).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/desloppify/SKILL.md:
- Around line 183-189: The fenced code block that begins with "You are a code
quality reviewer. You will be given a codebase path, a set of" lacks a language
identifier, causing linter warnings; update that fenced block in SKILL.md to
include a language token (e.g., ```text or ```markdown) immediately after the
opening backticks so the block becomes ```text (or ```markdown) and preserves
the existing content and indentation.
- Line 17: Update the cycle description in SKILL.md to include the mandatory
"next" step: change the sequence from "scan → plan → execute → rescan" to "scan
→ next → plan → execute → rescan" and explicitly mention running the command
desloppify next immediately after scanning; ensure the text aligns with the
existing "After scanning, always run desloppify next" guidance so the scan step
and the INSTRUCTIONS FOR AGENTS flow are consistent.
In `@adapter/zerolog/sink.go`:
- Around line 85-124: The appendField function can panic when a typed-nil error
(e.g., (*MyErr)(nil)) is matched by the "case error:" branch and val.Error() is
called; update appendField to detect nil underlying error values before calling
Error(): inside the error case in appendField, use reflect.ValueOf(val) to check
for a nil underlying pointer (e.g., if val == nil || (rv :=
reflect.ValueOf(val); rv.Kind() == reflect.Ptr && rv.IsNil()) ) and in that nil
case fallback to event.Interface(key, value); otherwise call val.Error() as
before; ensure you import reflect and only perform the IsNil check when safe
(ptr kinds).
In `@cmd/examples/integration_consistency_test.go`:
- Around line 385-392: panicDetails currently casts the panic metadata "value"
to string and returns empty when it's not a string, which hides non-string panic
values; update panicDetails to detect when field["value"] is not a string and
convert it to a stable string representation (e.g., use fmt.Sprintf("%v", v) or
JSON marshal of the value) so panicValue always contains a meaningful
representation, while still extracting "type" from the map as before; ensure the
function signature and returned variables (typ, panicValue) are preserved and
only the extraction/conversion logic in panicDetails is changed.
In `@integration/fiberv3/middleware.go`:
- Around line 51-60: The middleware currently unconditionally sets err = nil
after invoking the configured ErrorHandler, which swallows original handler
errors when no ErrorHandler is present; change the logic in the error handling
block so that you only clear err when an ErrorHandler exists and it returns nil
(i.e., the error was successfully processed), otherwise preserve the original
err or set finalizeErr to the handler's returned error (handlerErr) and return
that; reference c.App().Config().ErrorHandler, handlerErr, finalizeErr and
ensure the function returns finalizeErr if set, otherwise the preserved err.
In `@Justfile`:
- Around line 26-30: The module test loop that iterates over modfile via while
IFS= read -r modfile; do ... done does not preserve fail-fast semantics; if an
early (cd "$moddir" && go test ./...) fails the loop keeps going and the script
can return success. Modify the loop to capture each test’s exit code (from the
subshell that runs cd "$moddir" && go test ./...), set a failure flag or
accumulate a non-zero exit status when any test fails, and after the loop exit
with that aggregated non-zero status (e.g., track a variable like tests_failed
or overall_exit and call exit $overall_exit at the end) so CI fails if any
module tests fail.
In `@operation_lifecycle.go`:
- Around line 60-77: Operation.End can be called multiple times and will emit
duplicate terminal events; add a guard to ensure finalization runs only once by
introducing a per-operation guard (e.g., a sync.Once field or an atomic.Bool) on
the Operation struct and using it at the start of End to return false
immediately if already finalized; update End to only call finishOperation on the
first invocation (still preserve panic re-raise behavior for recovered) and add
a small regression test that calls Operation.End twice to assert only one
terminal event is written.
- Around line 211-217: The panic branch currently wraps recovered with
fmt.Errorf which loses concrete error types before structuredErrorField runs;
update the switch handling around errorField so that if recovered implements
error (check via e, ok := recovered.(error)) you pass e directly to
structuredErrorField, and only synthesize a new error (e.g. fmt.Errorf("panic:
%v", recovered)) when recovered is not an error; keep the same variables
(errorField, structuredErrorField, recovered, err) and behavior for the err !=
nil case.
---
Nitpick comments:
In @.agents/skills/desloppify/SKILL.md:
- Line 13: Change the British spelling "Maximise" to the American spelling
"Maximize" in the SKILL.md content for consistency (search for the token
"Maximise" and replace it with "Maximize"); ensure any occurrences in headings,
examples, or descriptive text related to the "Desloppify" skill are updated and
run a quick spellcheck to catch any additional British variants.
In `@cmd/examples/adapter-slog/main_test.go`:
- Around line 77-86: The "request with debug flag" subtest resets buf and issues
GET /users/789?debug=1 but never asserts that debug logging occurred; update
that subtest to assert buf.String() contains the expected debug-level marker
(e.g. "level=debug" or the specific debug message produced by handler) after
handler.ServeHTTP(rec, req), and similarly update the failure subtest to reset
buf, invoke the failing request, then assert buf.String() contains the expected
error/failure log (e.g. the error text or "level=error"). Use the existing
variables buf and handler to locate the code and keep assertions resilient by
matching substrings rather than full timestamps.
In `@cmd/examples/router-gin/main_test.go`:
- Around line 20-21: The test currently creates the logger with
slog.NewJSONHandler(os.Stdout, nil) and cannot assert log output; replace the
stdout sink with a bytes.Buffer (create buf := &bytes.Buffer{} and pass it to
slog.NewJSONHandler) and construct the sloghc logger from that buffer
(sloghc.New(logger)), then in each subtest call buf.Reset() before exercising
the middleware and after ServeHTTP inspect buf.String() and add assertions
(e.g., check strings.Contains(output, "router") or other expected fields) to
verify middleware logging behavior; update references to logger and sloghc.New
in main_test.go accordingly.
In `@cmd/examples/sampling-custom/main_test.go`:
- Around line 32-36: The test currently calls the local customSampler directly
and ignores the assembled config; update the assertions to invoke the sampler
via cfg.Sampler (where cfg is the hc.Config instance constructed with Sink,
Sampler: customSampler, Message) so you validate the wiring through hc.Config
rather than only the standalone function; replace direct calls to customSampler
with cfg.Sampler in both the first occurrence and the later similar block (the
other test around the same file), and keep existing expectations/assertions
identical (including any returned values or error checks) to ensure behavior is
unchanged while verifying the config wiring.
In `@event_access_test.go`:
- Around line 92-113: Add a doc comment to the public function EventFields
explaining it returns a shallow copy of the event's fields: mutations to
top-level keys on the returned map do not affect the original event, but nested
reference types (e.g., map[string]any, slices, pointers) are shared by reference
and can be mutated through the returned map; update the comment near the
EventFields declaration to state this behavior and its implications for callers.
In `@event_test.go`:
- Around line 116-130: The test TestSetErrorNormalizesFrameworkStyleErrors
relies on setError extracting the Message field from frameworkStyleError (rather
than using frameworkStyleError.Error() which returns "code=500, message=boom");
add a short clarifying comment above the test (or inside it) stating that
setError intentionally normalizes structured frameworkStyleError values by
reading the Message field and setting the structured "error" map["message"] to
that value so future maintainers understand why the raw Error() string is not
used; reference the setError method and the frameworkStyleError type in the
comment for clarity.
In `@integration/worker/worker_test.go`:
- Around line 82-103: The panic subtest currently assumes sink.Events() has at
least one element and directly uses sink.Events()[0]; add a guard to check the
events slice length (from sink.Events()) before indexing so the test fails with
a clear message if no events were recorded. Locate the subtest using Start,
JobMeta, hc.NewTestSink, op.End and hc.Config, call events := sink.Events(),
assert len(events) > 0 (or t.Fatalf with a helpful message) before accessing
events[0], then continue the existing assertions against events[0] and
hc.OutcomePanic and the "panic" field.
In `@integration/worker/worker.go`:
- Around line 34-45: The current addJobFields builds a kv slice then uses
kv[0].(string) which is unnecessary; instead call hc.Add directly with the
key/value pairs (e.g. hc.Add(ctx, "job.name", meta.Name, "job.id", meta.ID,
"job.queue", meta.Queue, "job.attempt", meta.Attempt, "job.max_attempts",
meta.MaxAttempts)), and handle the optional scheduled time by calling
hc.Add(ctx, "job.scheduled_at", meta.ScheduledAt.UTC()) only when
meta.ScheduledAt.IsZero() is false; update the addJobFields function to remove
the kv slice and type assertion and use these direct variadic hc.Add calls.
In `@metadata.go`:
- Around line 26-31: structuredPanicField should handle a nil recovered value
like structuredErrorField does; add a nil check at the start of
structuredPanicField (function name: structuredPanicField) and when recovered ==
nil return a map with "type" set to "nil" and "value" set to nil (or the same
representation used in structuredErrorField) otherwise proceed to build the
existing map using fmt.Sprintf and fmt.Sprint.
In `@operation_lifecycle.go`:
- Around line 91-100: finishOperation currently reapplies OperationStart fields
into ctx via applyOperationStartFields(ctx, start) but then mutates and emits
the separate event variable, which can cause mismatched/incorrect output if ctx
and event differ; update finishOperation so the operation start fields are
applied directly to the event before computing outcome and persisting (i.e.,
replace applyOperationStartFields(ctx, start) with applying the same
start-to-event mapping to the event object or add/use an
applyOperationStartFieldsToEvent(event, start) helper), or alternatively
validate that ctx and event reference the same underlying object and fail fast;
ensure all subsequent writes (resolveOutcome, final sink write) use that single
event instance (references: finishOperation, applyOperationStartFields, event,
OperationStart, policyForDomain, resolveOutcome).
In `@operation_policy.go`:
- Around line 8-9: The condition uses both in.Code and in.StatusCode alongside
in.HasError and in.Outcome != OutcomeSuccess, which is ambiguous; either
consolidate to a single status field or add a short clarifying comment and/or
rename fields to show their distinct meanings. Update the boolean check around
in.HasError/in.Code/in.StatusCode/in.Outcome (the expression containing in.Code
>= 500 and in.StatusCode >= 500) to remove redundancy if they represent the same
HTTP status, or keep both but add a comment above the condition explaining that
Code is an operation-specific code while StatusCode is the HTTP response code
(or vice versa), and adjust any names or logic in the function that references
these fields to match the clarified semantics.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 066d1150-db20-4b90-a113-e431ed3a1f0c
⛔ Files ignored due to path filters (2)
assets/demo-before-after.svgis excluded by!**/*.svgscorecard.pngis excluded by!**/*.png
📒 Files selected for processing (79)
.agents/skills/desloppify/SKILL.md.gitignoreJustfileREADME.mdadapter/slog/benchmark_test.goadapter/slog/sink.goadapter/slog/sink_test.goadapter/zap/benchmark_test.goadapter/zap/sink.goadapter/zap/sink_test.goadapter/zerolog/benchmark_test.goadapter/zerolog/sink.goadapter/zerolog/sink_test.gobench/happycontext_benchmark_test.gocmd/examples/README.mdcmd/examples/adapter-slog/main.gocmd/examples/adapter-slog/main_test.gocmd/examples/adapter-zap/main.gocmd/examples/adapter-zap/main_test.gocmd/examples/adapter-zerolog/main.gocmd/examples/adapter-zerolog/main_test.gocmd/examples/go.modcmd/examples/integration_consistency_test.gocmd/examples/router-echo/main.gocmd/examples/router-echo/main_test.gocmd/examples/router-fiber/main.gocmd/examples/router-fiber/main_test.gocmd/examples/router-fiberv3/main.gocmd/examples/router-fiberv3/main_test.gocmd/examples/router-gin/main.gocmd/examples/router-gin/main_test.gocmd/examples/router-std/main.gocmd/examples/router-std/main_test.gocmd/examples/sampling-custom/main.gocmd/examples/sampling-custom/main_test.gocmd/examples/sampling-inbuilt/main.gocmd/examples/sampling-inbuilt/main_test.gocmd/examples/worker-job/main.gocmd/examples/worker-job/main_test.goconfig.goconfig_test.gocontext.gocontext_test.goevent.goevent_access_test.goevent_test.gohappycontext.gohappycontext_test.gointegration/common/config.gointegration/common/config_test.gointegration/common/lifecycle.gointegration/common/lifecycle_test.gointegration/common/sampling.gointegration/common/sampling_test.gointegration/echo/middleware.gointegration/echo/middleware_test.gointegration/fiber/middleware.gointegration/fiber/middleware_test.gointegration/fiberv3/middleware.gointegration/fiberv3/middleware_test.gointegration/gin/middleware.gointegration/gin/middleware_test.gointegration/std/middleware.gointegration/std/middleware_test.gointegration/worker/go.modintegration/worker/worker.gointegration/worker/worker_test.golevel.golevel_test.gometadata.gometadata_test.gooperation_lifecycle.gooperation_policy.gooperation_policy_test.gooperation_test.gooperation_types.gooperation_types_test.gooptions.gosampling.go
💤 Files with no reviewable changes (3)
- integration/common/lifecycle_test.go
- integration/common/sampling.go
- integration/common/sampling_test.go
Comment on lines
+385
to
+392
| func panicDetails(value any) (typ, panicValue string) { | ||
| field, ok := value.(map[string]any) | ||
| if !ok { | ||
| return "", "" | ||
| } | ||
| typ, _ = field["type"].(string) | ||
| panicValue, _ = field["value"].(string) | ||
| return typ, panicValue |
There was a problem hiding this comment.
panicDetails drops non-string panic values.
If panic metadata value is not a string, this returns "" and can hide cross-integration differences in this consistency test.
Proposed fix
+import "fmt"
+
func panicDetails(value any) (typ, panicValue string) {
field, ok := value.(map[string]any)
if !ok {
return "", ""
}
typ, _ = field["type"].(string)
- panicValue, _ = field["value"].(string)
+ if v, exists := field["value"]; exists && v != nil {
+ panicValue = fmt.Sprint(v)
+ }
return typ, panicValue
}📝 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
| func panicDetails(value any) (typ, panicValue string) { | |
| field, ok := value.(map[string]any) | |
| if !ok { | |
| return "", "" | |
| } | |
| typ, _ = field["type"].(string) | |
| panicValue, _ = field["value"].(string) | |
| return typ, panicValue | |
| func panicDetails(value any) (typ, panicValue string) { | |
| field, ok := value.(map[string]any) | |
| if !ok { | |
| return "", "" | |
| } | |
| typ, _ = field["type"].(string) | |
| if v, exists := field["value"]; exists && v != nil { | |
| panicValue = fmt.Sprint(v) | |
| } | |
| return typ, panicValue | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/examples/integration_consistency_test.go` around lines 385 - 392,
panicDetails currently casts the panic metadata "value" to string and returns
empty when it's not a string, which hides non-string panic values; update
panicDetails to detect when field["value"] is not a string and convert it to a
stable string representation (e.g., use fmt.Sprintf("%v", v) or JSON marshal of
the value) so panicValue always contains a meaningful representation, while
still extracting "type" from the map as before; ensure the function signature
and returned variables (typ, panicValue) are preserved and only the
extraction/conversion logic in panicDetails is changed.
Comment on lines
+26
to
+30
| while IFS= read -r modfile; do \ | ||
| moddir="$(dirname "$modfile")"; \ | ||
| echo "== testing $moddir =="; \ | ||
| (cd "$moddir" && go test ./...); \ | ||
| done < <(printf '%s\n' go.mod && git ls-files '**/go.mod') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect test recipe block:"
sed -n '25,33p' Justfile
echo
echo "Check whether failure status is explicitly aggregated:"
rg -n 'status=|exit "\$status"|set -e|\|\| status=' Justfile || trueRepository: happytoolin/happycontext
Length of output: 399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check shell configuration at top of Justfile
echo "=== Shell configuration in Justfile ==="
head -20 Justfile
echo
echo "=== Full test recipe context ==="
sed -n '24,35p' Justfile
echo
echo "=== Test bash syntax: status=\$\$? vs status=\$? ==="
# Create a test function to check both syntaxes
cat > /tmp/test_syntax.sh << 'TESTEOF'
#!/bin/bash
# Test what $$? evaluates to
echo "Testing: status=\$\$?"
status=$$?
echo "Result: status='$status'"
echo "Expected: should fail or produce unexpected value (not exit code)"
echo
echo "Testing: status=\$? (with a failing command)"
(exit 5) || status=$?
echo "Result: status='$status'"
echo "Expected: status should be '5'"
TESTEOF
chmod +x /tmp/test_syntax.sh
/tmp/test_syntax.sh || trueRepository: happytoolin/happycontext
Length of output: 1193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Testing the CORRECTED fix syntax in ZSH ==="
# Test the corrected fix in ZSH context
zsh << 'ZSHEOF'
set -euo pipefail
# Simulate the corrected fix logic
status=0
echo "Test 1: Module passes, next module fails"
(exit 0) || status=$?
echo " After pass: status=$status"
(exit 42) || status=$?
echo " After fail: status=$status"
echo
echo "Test 2: Early module fails, later passes"
status=0
(exit 5) || status=$?
echo " After fail: status=$status"
(exit 0) || status=$?
echo " After pass: status=$status"
echo
echo "Conclusion: With \`|| status=\$?\`, the accumulated status persists correctly"
ZSHEOF
echo
echo "=== Verify the incorrect syntax (\$\$?) also fails in ZSH ==="
zsh << 'ZSHEOF2'
status=$$?
echo "Result with \$\$?: '$status' (should be wrong)"
ZSHEOF2Repository: happytoolin/happycontext
Length of output: 176
Fix fail-fast semantics in the module test loop.
The test recipe at lines 26–30 does not aggregate exit codes. When an early module's test fails, the loop continues and can exit successfully if a later module passes, masking failures and producing false-green CI.
Proposed fix
test:
+ status=0; \
while IFS= read -r modfile; do \
moddir="$(dirname "$modfile")"; \
echo "== testing $moddir =="; \
- (cd "$moddir" && go test ./...); \
+ (cd "$moddir" && go test ./...) || status=$?; \
done < <(printf '%s\n' go.mod && git ls-files '**/go.mod' | sort -u); \
+ exit "$status"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Justfile` around lines 26 - 30, The module test loop that iterates over
modfile via while IFS= read -r modfile; do ... done does not preserve fail-fast
semantics; if an early (cd "$moddir" && go test ./...) fails the loop keeps
going and the script can return success. Modify the loop to capture each test’s
exit code (from the subshell that runs cd "$moddir" && go test ./...), set a
failure flag or accumulate a non-zero exit status when any test fails, and after
the loop exit with that aggregated non-zero status (e.g., track a variable like
tests_failed or overall_exit and call exit $overall_exit at the end) so CI fails
if any module tests fail.
Comment on lines
+60
to
+77
| func (op *Operation) End(cfg Config, errp *error) bool { | ||
| if op == nil { | ||
| return false | ||
| } | ||
| var err error | ||
| if errp != nil { | ||
| err = *errp | ||
| } | ||
| recovered := recover() | ||
| wrote := finishOperation(cfg, op.ctx, op.event, op.start, operationResult{ | ||
| Err: err, | ||
| Recovered: recovered, | ||
| }) | ||
| if recovered != nil { | ||
| panic(recovered) | ||
| } | ||
| return wrote | ||
| } |
There was a problem hiding this comment.
Guard Operation.End against double finalization.
End is currently single-call in convention only: invoking it twice on the same handle will write two terminal events because nothing records that the operation was already finalized. For a stateful lifecycle API that will skew counts and duplicate failure telemetry. A sync.Once/atomic.Bool guard here would make the handle safe, and this is worth covering with a small regression test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operation_lifecycle.go` around lines 60 - 77, Operation.End can be called
multiple times and will emit duplicate terminal events; add a guard to ensure
finalization runs only once by introducing a per-operation guard (e.g., a
sync.Once field or an atomic.Bool) on the Operation struct and using it at the
start of End to return false immediately if already finalized; update End to
only call finishOperation on the first invocation (still preserve panic re-raise
behavior for recovered) and add a small regression test that calls Operation.End
twice to assert only one terminal event is written.
Comment on lines
+211
to
+217
| var errorField map[string]any | ||
| switch { | ||
| case err != nil: | ||
| errorField = structuredErrorField(err) | ||
| case recovered != nil: | ||
| errorField = structuredErrorField(fmt.Errorf("panic: %v", recovered)) | ||
| } |
There was a problem hiding this comment.
Preserve concrete panic error types here.
When recovered already implements error, wrapping it with fmt.Errorf("panic: %v", recovered) collapses it into a generic formatted error before structuredErrorField sees it. That loses the concrete type metadata this PR is trying to preserve. Pass through the original error when possible and only synthesize a string error for non-error panic values.
🔧 Proposed fix
switch {
case err != nil:
errorField = structuredErrorField(err)
case recovered != nil:
- errorField = structuredErrorField(fmt.Errorf("panic: %v", recovered))
+ if recoveredErr, ok := recovered.(error); ok {
+ errorField = structuredErrorField(recoveredErr)
+ } else {
+ errorField = structuredErrorField(fmt.Errorf("panic: %v", recovered))
+ }
}📝 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
| var errorField map[string]any | |
| switch { | |
| case err != nil: | |
| errorField = structuredErrorField(err) | |
| case recovered != nil: | |
| errorField = structuredErrorField(fmt.Errorf("panic: %v", recovered)) | |
| } | |
| var errorField map[string]any | |
| switch { | |
| case err != nil: | |
| errorField = structuredErrorField(err) | |
| case recovered != nil: | |
| if recoveredErr, ok := recovered.(error); ok { | |
| errorField = structuredErrorField(recoveredErr) | |
| } else { | |
| errorField = structuredErrorField(fmt.Errorf("panic: %v", recovered)) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operation_lifecycle.go` around lines 211 - 217, The panic branch currently
wraps recovered with fmt.Errorf which loses concrete error types before
structuredErrorField runs; update the switch handling around errorField so that
if recovered implements error (check via e, ok := recovered.(error)) you pass e
directly to structuredErrorField, and only synthesize a new error (e.g.
fmt.Errorf("panic: %v", recovered)) when recovered is not an error; keep the
same variables (errorField, structuredErrorField, recovered, err) and behavior
for the err != nil case.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/release.yml:
- Around line 27-30: The Checkout step currently uses the tag ref
actions/checkout@v6.0.2 and leaves credential persistence enabled; update the
Checkout step (the "Checkout" job step that uses actions/checkout@v6.0.2) to pin
the action to a full commit SHA instead of the tag and add persist-credentials:
false to the step inputs so the workflow does not expose write tokens during git
operations (noting the workflow has permissions: contents: write and later
performs git push).
In `@scripts/publish-lockstep-release.sh`:
- Around line 95-96: create_release_if_missing currently always runs the gh
release create command (the line invoking gh release create "$tag_name" --title
"$title" --notes "$notes" --verify-tag) even when new tags were only created
locally and the script was not run with --push; modify create_release_if_missing
to guard that call so it only runs when either no new local tags were created
(created_tags is empty) or the run included pushing tags (the --push flag / push
variable is true); otherwise skip calling gh release create (or defer/remove
--verify-tag) and log a clear message indicating releases were not created
because tags were not pushed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb05a393-b8ae-489a-ad1d-fbb058be3d4a
📒 Files selected for processing (38)
.github/workflows/go-proxy-sync.yml.github/workflows/release.yml.gitignore.release-please-config.json.release-please-manifest.jsonREADME.mdadapter/slog/benchmark_test.goadapter/slog/go.modadapter/slog/sink.goadapter/slog/sink_test.goadapter/zap/benchmark_test.goadapter/zap/go.modadapter/zap/sink.goadapter/zap/sink_test.goadapter/zerolog/benchmark_test.goadapter/zerolog/go.modadapter/zerolog/sink.goadapter/zerolog/sink_test.gobench/go.modcmd/examples/go.modcmd/examples/sampling-custom/main_test.gointegration/echo/go.modintegration/echo/middleware.gointegration/fiber/go.modintegration/fiber/middleware.gointegration/fiberv3/go.modintegration/fiberv3/middleware.gointegration/gin/go.modintegration/gin/middleware.gointegration/gin/middleware_test.gointegration/std/go.modintegration/std/middleware.gointegration/std/middleware_test.gointegration/worker/go.modoptions.goscripts/backfill-go-tags.shscripts/publish-lockstep-release.shscripts/sync-lockstep-release.sh
✅ Files skipped from review due to trivial changes (10)
- .release-please-config.json
- bench/go.mod
- integration/fiberv3/go.mod
- integration/echo/go.mod
- .release-please-manifest.json
- adapter/zerolog/go.mod
- integration/std/go.mod
- integration/fiber/go.mod
- adapter/slog/benchmark_test.go
- README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/examples/go.mod
- integration/worker/go.mod
- cmd/examples/sampling-custom/main_test.go
- .gitignore
- options.go
Comment on lines
+27
to
+30
| - name: Checkout | ||
| if: ${{ steps.release.outputs.releases_created == 'true' }} | ||
| uses: actions/checkout@v6.0.2 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/release.yml"
echo "== file exists =="
ls -la "$FILE"
echo "== lines 1-120 =="
nl -ba "$FILE" | sed -n '1,140p'
echo "== checkout-related lines =="
nl -ba "$FILE" | sed -n '20,60p'
echo "== search persist-credentials, permissions, contents write =="
rg -n "persist-credentials" .github/workflows -S || true
rg -n "permissions:" .github/workflows/release.yml -S || true
rg -n "contents:" .github/workflows/release.yml -S || true
rg -n "actions/checkout@" .github/workflows/release.yml -S || trueRepository: happytoolin/happycontext
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/release.yml"
echo "== checkout-related section =="
echo "--- cat -n 1-120 ---"
cat -n "$FILE" | sed -n '1,140p'
echo "== checkout-related lines 20-60 =="
cat -n "$FILE" | sed -n '20,70p'
echo "== search persist-credentials, permissions, contents write =="
rg -n "persist-credentials" .github/workflows -S || true
rg -n "permissions:" .github/workflows/release.yml -S || true
rg -n "contents:" .github/workflows/release.yml -S || true
rg -n "actions/checkout@" .github/workflows/release.yml -S || trueRepository: happytoolin/happycontext
Length of output: 6807
Harden checkout step: pin SHA and disable credential persistence.
In .github/workflows/release.yml, the Checkout step uses actions/checkout@v6.0.2 (tag ref, not a full commit SHA) and does not set persist-credentials: false while the workflow has permissions: contents: write and later performs git push, which increases token exposure risk.
Suggested hardening
- - name: Checkout
+ - name: Checkout
if: ${{ steps.release.outputs.releases_created == 'true' }}
- uses: actions/checkout@v6.0.2
+ uses: actions/checkout@<full_commit_sha>
+ with:
+ persist-credentials: false🧰 Tools
🪛 zizmor (1.25.2)
[warning] 27-29: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 29-29: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/release.yml around lines 27 - 30, The Checkout step
currently uses the tag ref actions/checkout@v6.0.2 and leaves credential
persistence enabled; update the Checkout step (the "Checkout" job step that uses
actions/checkout@v6.0.2) to pin the action to a full commit SHA instead of the
tag and add persist-credentials: false to the step inputs so the workflow does
not expose write tokens during git operations (noting the workflow has
permissions: contents: write and later performs git push).
Comment on lines
+95
to
+96
| gh release create "$tag_name" --title "$title" --notes "$notes" --verify-tag | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="scripts/publish-lockstep-release.sh"
echo "=== File exists? ==="
ls -l "$FILE"
echo "=== Snippet around lines 70-130 ==="
nl -ba "$FILE" | sed -n '70,140p'
echo "=== Search for key identifiers ==="
rg -n --hidden --no-ignore-vcs "github-releases|create_releases|push_tags|created_tags|verify-tag|gh release create" "$FILE" || trueRepository: happytoolin/happycontext
Length of output: 271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="scripts/publish-lockstep-release.sh"
echo "=== File exists? ==="
ls -l "$FILE"
echo "=== Snippet around lines 70-130 ==="
nl -ba "$FILE" | sed -n '70,140p'
echo "=== Search for key identifiers ==="
rg -n --hidden --no-ignore-vcs "github-releases|create_releases|push_tags|created_tags|verify-tag|gh release create" "$FILE" || trueRepository: happytoolin/happycontext
Length of output: 271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="scripts/publish-lockstep-release.sh"
echo "=== Snippet around lines 70-130 (cat -n) ==="
sed -n '70,130p' "$FILE" | cat -n -w3 -s' ' | sed -n '1,200p'
echo "=== Search for key identifiers ==="
# use grep (likely available) instead of rg if needed; but try rg first
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore-vcs "github-releases|create_releases|push_tags|created_tags|verify-tag|gh release create" "$FILE" || true
else
grep -nE "github-releases|create_releases|push_tags|created_tags|verify-tag|gh release create" "$FILE" || true
fiRepository: happytoolin/happycontext
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="scripts/publish-lockstep-release.sh"
echo "=== Snippet around lines 70-130 (awk line numbers) ==="
awk 'NR>=70 && NR<=130 {printf "%4d %s\n", NR, $0}' "$FILE"
echo "=== Snippet around lines 85-120 ==="
awk 'NR>=85 && NR<=120 {printf "%4d %s\n", NR, $0}' "$FILE"
echo "=== Search for key identifiers ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore-vcs "github-releases|create_releases|push_tags|created_tags|verify-tag|gh release create" "$FILE" || true
else
grep -nE "github-releases|create_releases|push_tags|created_tags|verify-tag|gh release create" "$FILE" || true
fiRepository: happytoolin/happycontext
Length of output: 3292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="scripts/publish-lockstep-release.sh"
echo "=== File head (1-90) ==="
awk 'NR>=1 && NR<=90 {printf "%4d %s\n", NR, $0}' "$FILE"
echo "=== Search for git fetch and tag-related fetch ==="
grep -nE "git fetch|fetch --tags|--tags|tag -a|rev-parse" "$FILE" || trueRepository: happytoolin/happycontext
Length of output: 2576
Guard --github-releases when new tags were created locally but --push wasn’t used.
create_release_if_missing uses gh release create ... --verify-tag, while the script only git pushes created_tags when --push is set—so release creation can fail when this run created new tags but didn’t push them.
Guardrail option
+if [[ "$create_releases" == true && "$push_tags" != true && ${`#created_tags`[@]} -gt 0 ]]; then
+ echo "--github-releases requires --push when creating new tags" >&2
+ exit 1
+fi
+
if [[ "$create_releases" == true ]]; then🤖 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 `@scripts/publish-lockstep-release.sh` around lines 95 - 96,
create_release_if_missing currently always runs the gh release create command
(the line invoking gh release create "$tag_name" --title "$title" --notes
"$notes" --verify-tag) even when new tags were only created locally and the
script was not run with --push; modify create_release_if_missing to guard that
call so it only runs when either no new local tags were created (created_tags is
empty) or the run included pushing tags (the --push flag / push variable is
true); otherwise skip calling gh release create (or defer/remove --verify-tag)
and log a clear message indicating releases were not created because tags were
not pushed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@metadata_test.go`:
- Around line 75-76: The 100ms timeout in the test's select is too short and
causes flaky failures; update the timeout from 100 * time.Millisecond to a
larger value (e.g., 1 * time.Second) where the select checks for
structuredErrorField returning for cyclic unwrap so the test waits longer under
CI load; locate the select that references structuredErrorField in
metadata_test.go and replace the 100ms duration with 1s.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 584d6f9e-516d-489b-ab43-f978a2c01cbe
📒 Files selected for processing (13)
adapter/zap/sink_test.goadapter/zerolog/sink_test.gobench/integration/router_benchmark_test.gocmd/examples/router-fiber/main_test.gocmd/examples/router-fiberv3/main_test.gointegration/fiber/middleware.gointegration/fiber/middleware_test.gointegration/fiberv3/middleware.gointegration/fiberv3/middleware_test.gometadata.gometadata_test.gooperation_lifecycle.gooperation_test.go
✅ Files skipped from review due to trivial changes (1)
- bench/integration/router_benchmark_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- adapter/zap/sink_test.go
- adapter/zerolog/sink_test.go
- integration/fiber/middleware.go
- cmd/examples/router-fiber/main_test.go
- metadata.go
- cmd/examples/router-fiberv3/main_test.go
- operation_lifecycle.go
Comment on lines
+75
to
+76
| case <-time.After(100 * time.Millisecond): | ||
| t.Fatal("structuredErrorField did not return for cyclic unwrap") |
There was a problem hiding this comment.
Relax the cyclic-unwrap timeout to avoid flaky failures.
Line 75 uses a 100ms timeout, which is brittle under loaded CI and can fail even when structuredErrorField is correct. Use a larger timeout (for example, 1s).
Suggested patch
- case <-time.After(100 * time.Millisecond):
+ case <-time.After(1 * time.Second):
t.Fatal("structuredErrorField did not return for cyclic unwrap")📝 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
| case <-time.After(100 * time.Millisecond): | |
| t.Fatal("structuredErrorField did not return for cyclic unwrap") | |
| case <-time.After(1 * time.Second): | |
| t.Fatal("structuredErrorField did not return for cyclic unwrap") |
🤖 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 `@metadata_test.go` around lines 75 - 76, The 100ms timeout in the test's
select is too short and causes flaky failures; update the timeout from 100 *
time.Millisecond to a larger value (e.g., 1 * time.Second) where the select
checks for structuredErrorField returning for cyclic unwrap so the test waits
longer under CI load; locate the select that references structuredErrorField in
metadata_test.go and replace the 100ms duration with 1s.
# Conflicts: # .github/workflows/release.yml # scripts/sync-lockstep-release.sh
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 free
to 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
just testcover all nested modulesTesting
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests