Pr/har logging#1946
Merged
Merged
Conversation
Contributor
WalkthroughAdds HTTP observability/HAR middleware and context APIs; provides properties documentation and JSON schema; expands scraper-scoped DB/model keys and view logic; adds MergeConfigTrees and related query changes; updates transport wiring (HTTP/Git/K8s); adds tests and tooling/docs. ChangesHTTP Observability & HAR Collection Middleware
Scraper Context & DB/View Changes
Config Tree & Query Enhancements
Properties Documentation & Schema
Tooling, Tests & Startup
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Benchstat (RLS)Base: 📊 3 minor regression(s) (all within 5% threshold)
✅ 6 improvement(s)
Full benchstat output |
Benchstat (Other)Base: 📊 2 minor regression(s) (all within 5% threshold)
Full benchstat output |
…r groups Introduce scraper_id as part of composite primary keys for ConfigRelationship and ExternalUserGroup to support multi-scraper ownership. This allows multiple scrapers to independently maintain relationship tuples without colliding, while uuid.Nil represents legacy scraper-agnostic relationships. Add Health.Badge() method to render health status as colored pills for dense table cells. Implement MergeConfigTrees() to collapse independently-built config trees into a unified forest by merging shared ancestors recursively. Enhance SearchResources to populate Health and Status fields for configs and components. Add RoleExternalIDs to ConfigAccessSummary. Create external_group_summary view with member and permission counts. Update SQL views to filter deleted relationships and add GROUP BY clauses for accurate counts. Simplify database setup helpers by removing unnecessary quoting and URL parsing utilities. BREAKING CHANGE: ConfigRelationship and ExternalUserGroup primary keys now include scraper_id; existing code must populate this field from scraper context.
Replace ginkgo-based test targets with gavel, a unified test runner that provides better output formatting and cross-platform compatibility. Update test and test-concurrent targets to use gavel with consistent timeout and ignore patterns. Add gavel installation target and captain target for properties documentation generation. Add PROPERTIES.md and PROPERTIES.schema.json documenting all runtime properties across duty and related services. Update Taskfile.yaml test task to use gavel with passthrough argument support.
…ty middleware into reusable functions Extract HTTP observability logic (HAR collection, HTTP logging) from individual connection types into shared middleware functions in connection/common.go. This eliminates code duplication across AWS, GCS, GKE, OpenSearch, and other connection implementations. New observability functions: - applyHTTPObservability: applies HAR and HTTP logging to a RoundTripper - httpObservabilityMiddleware: returns a middleware for WrapTransport - applyHTTPClientObservability: applies observability to commons HTTP clients - harCollectorMiddleware: HAR-specific middleware with level-aware behavior - httpLoggerWithContent: HTTP request/response logging with configurable detail - metadataHARMiddleware: HAR metadata-only capture for debug-level logging Add context methods for observability configuration: - EffectiveHARLevel, EffectiveLogLevel: resolve per-feature log levels - EffectiveHARCollector: select HAR collector with feature-aware config - HTTPLoggingContent: determine header/body logging for a feature - HARConfig: build feature-specific HAR configuration Update connection implementations to use shared functions, reducing per-connection boilerplate. Add git HTTP transport observability support via configureGitHTTPTransport. Add comprehensive tests for HAR and HTTP logging coexistence, ensuring body restoration across middleware layers. Refs #1234
yashmehrotra
force-pushed
the
pr/har-logging
branch
from
d710034 to
7de634a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
query/resource_selector.go (1)
293-322:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
agentIDis shadowed; the outer value passed togetScopeIDis always nil.At line 295,
agentID, err := getAgentID(...)re-declaresagentIDin the innerifblock, so the outeragentIDdeclared at line 293 staysnil. Consequently,getScopeID(ctx, ..., agentID)on line 306 never receives the resolved agent and the scope lookup is unscoped by agent. With the newsearchSetIDbranch this is more visible, since callers may now reach the scope path with different expectations about agent scoping.If this is intentional, a brief comment would help; otherwise the inner block should assign to the outer variable (
agentID, err = getAgentID(...)).🛠 Proposed fix
- var agentID *uuid.UUID - if !searchSetAgent && !searchSetID && qm.HasAgents { - agentID, err := getAgentID(ctx, resourceSelector.Agent) - if err != nil { - return nil, err - } - - if agentID != nil { - query = query.Where("agent_id = ?", *agentID) - } - } + var agentID *uuid.UUID + if !searchSetAgent && !searchSetID && qm.HasAgents { + agentID, err = getAgentID(ctx, resourceSelector.Agent) + if err != nil { + return nil, err + } + + if agentID != nil { + query = query.Where("agent_id = ?", *agentID) + } + }🤖 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 `@query/resource_selector.go` around lines 293 - 322, The agentID declared at the top of the block is being shadowed by the inner short-declaration (agentID, err := getAgentID(...)) inside the qm.HasAgents branch, so the outer agentID remains nil and the subsequent getScopeID(ctx, resourceSelector.Scope, table, agentID) call is unscoped; change the inner declaration to assign to the outer variable (use agentID, err = getAgentID(...)) so the resolved agent is passed into getScopeID, and ensure error handling remains the same; check the qm.HasAgents, searchSetAgent, and searchSetID logic around resourceSelector.Scope and table (checks/config_items/components/config_changes/catalog_changes) to confirm scoping behavior is preserved.tests/setup/common.go (1)
278-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQuote or validate the derived database name before interpolating it into SQL.
newNameincludes the caller-suppliedname, so these statements now break on valid-but-unquoted identifiers and reintroduce SQL-injection risk that the old helper avoided.🧱 Suggested fix
+func quoteIdentifier(name string) string { + return `"` + strings.ReplaceAll(name, `"`, `""`) + `"` +} + - if err := ctx.DB().Exec(fmt.Sprintf("CREATE DATABASE %s", newName)).Error; err != nil { + if err := ctx.DB().Exec("CREATE DATABASE " + quoteIdentifier(newName)).Error; err != nil { return nil, nil, err } @@ - if err := ctx.DB().Exec(fmt.Sprintf("DROP DATABASE %s (FORCE)", newName)).Error; err != nil { + if err := ctx.DB().Exec("DROP DATABASE " + quoteIdentifier(newName) + " (FORCE)").Error; err != nil { logger.Errorf("error cleaning up db: %v", err) } }, nil🤖 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 `@tests/setup/common.go` around lines 278 - 299, The DB creation/drop SQL interpolates caller-supplied newName directly (seen in the ctx.DB().Exec("CREATE DATABASE %s", newName) and DROP DATABASE ... (FORCE) calls), reintroducing SQL-injection risk; instead either validate newName against a strict identifier regex (e.g. ^[A-Za-z_][A-Za-z0-9_]*$) before use or quote it safely with a proper identifier-quoting helper (e.g. pq.QuoteIdentifier or your project's equivalent) and use that quoted value in the Exec calls; update the code paths around newName, the CREATE DATABASE and DROP DATABASE Exec invocations, to use the validated or quoted identifier.connection/http.go (1)
351-410:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
Transport/TransportWithContextdoes not honorHTTPConnection.TLSsettings.The Transport path constructs the base from
&netHTTP.Transport{}(line 357) without applying the connection's TLS settings (CA, Cert, Key,InsecureSkipVerify,HandshakeTimeout). The assignment at lines 397–399 is dead code —connis a value copy ofrt.HTTPConnection, sort.TLS = conn.TLSassigns the embedded field to itself without configuring the actual underlying transport. Contrast withCreateHTTPClient(lines 445–456), which correctly applies TLS viaclient.TLSConfig(...).A caller using
conn.TransportWithContext(ctx)with TLS settings — such asInsecureSkipVerify: truefor a self-signed certificate — will silently use Go's default TLS behavior and fail. The PrometheusConnection client at line 41 is directly affected if its underlying HTTPConnection has TLS settings.Apply TLS configuration to the base
netHTTP.Transportbefore wrapping with observability, and remove the no-op assignment fromRoundTrip:Suggested fix
func (h HTTPConnection) TransportWithContext(ctx any, opts ...types.ClientOption) netHTTP.RoundTripper { o := types.NewClientOptions(opts...) + tr := &netHTTP.Transport{} + if !h.TLS.IsEmpty() { + tr.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: h.TLS.InsecureSkipVerify, + // ... populate from h.TLS.CA, h.TLS.Cert, h.TLS.Key + } + if h.TLS.HandshakeTimeout != 0 { + tr.TLSHandshakeTimeout = h.TLS.HandshakeTimeout + } + } - base := applyHTTPObservability(ctx, "http", &netHTTP.Transport{}, o.HARCollector) + base := applyHTTPObservability(ctx, "http", tr, o.HARCollector) rt := &httpConnectionRoundTripper{ HTTPConnection: h, Base: base, TokenTransport: harTokenTransport(ctx, "http", o.HARCollector), } return rt }And remove lines 397–399.
🤖 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 `@connection/http.go` around lines 351 - 410, TransportWithContext currently constructs base as &netHTTP.Transport{} via applyHTTPObservability without applying HTTPConnection.TLS, so TLS settings are ignored and the later rt.TLS = conn.TLS in httpConnectionRoundTripper.RoundTrip is a no-op; update TransportWithContext (the Transport and TransportWithContext functions) to take conn := h (or access h.TLS) and apply conn.TLS into the underlying netHTTP.Transport (CA, Cert/Key, InsecureSkipVerify, HandshakeTimeout etc.) before calling applyHTTPObservability/creating base, mirroring how CreateHTTPClient/client.TLSConfig applies TLS, and remove the dead assignment in httpConnectionRoundTripper.RoundTrip that sets rt.TLS = conn.TLS.
🧹 Nitpick comments (13)
models/common.go (1)
91-95: 💤 Low valueRedundant
HealthUnknowncase duplicates the default branch.The
HealthUnknowncase returns the exact same badge as thedefaultbranch. Either drop the explicit case, or differentiateHealthUnknown(e.g. with distinct styling, asPretty()does) so the case earns its keep.♻️ Proposed simplification
case HealthWarning: return api.Badge(label, "bg-yellow-100", "text-yellow-800", "capitalize") - case HealthUnknown: - return api.Badge(label, "bg-gray-100", "text-gray-600", "capitalize") default: return api.Badge(label, "bg-gray-100", "text-gray-600", "capitalize") }🤖 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 `@models/common.go` around lines 91 - 95, The HealthUnknown case in the badge switch duplicates the default branch in the function that returns api.Badge (located in models/common.go), so remove the explicit "case HealthUnknown:" branch to rely on the default branch, or if you want a distinct presentation keep the case but change its styling to differ from default (e.g., use the same styling as Pretty() or a unique bg/text class) and update the api.Badge call in that branch accordingly.query/config_tree.go (3)
35-98: ⚡ Quick winDocument that the merged forest is a DAG (shared node pointers across roots).
cloneConfigTreereturns the existing pointer when an ID is re-encountered, so the same*ConfigTreeNodemay appear under multiple roots — this is exercised byTestMergeConfigTreesSharedInternalNode(BeIdenticalTo). Callers that mutate, JSON-serialize, or walk the result without avisitedset may be surprised: mutations propagate across roots, and serializers will emit the shared subtree once per parent. The current docstring talks about "trees" and "forest" but doesn't call this out.A short note in the
MergeConfigTreesdocstring would prevent misuse.📝 Suggested doc tweak
// EdgeType="target" is preserved when the same node appears as a target in one // input and as a non-target (parent/child/related) in another. +// +// The returned forest is a DAG: a node that appears under multiple roots is +// aliased (same *ConfigTreeNode pointer), not deep-copied. Callers that mutate +// or walk the result must be aware that subtrees can be shared across roots. func MergeConfigTrees(trees []*ConfigTreeNode) []*ConfigTreeNode {🤖 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 `@query/config_tree.go` around lines 35 - 98, Update the MergeConfigTrees docstring to explicitly state that the returned "forest" can be a DAG with shared *ConfigTreeNode pointers across roots (because cloneConfigTree returns existing nodes from byID), so callers should not assume disjoint trees; note that mutations to a shared node will affect all roots that reference it, JSON serializers may duplicate the shared subtree under each parent, and walkers should use a visited set to avoid repeated processing (refer to MergeConfigTrees, cloneConfigTree, and the behavior exercised by TestMergeConfigTreesSharedInternalNode).
81-98: 💤 Low value
Relationfromsrcis silently dropped whendst.Relationdiffers.
mergeConfigTreeIntoonly promotesEdgeTypeto"target"; it never touchesRelation. If the same node is reached via different relations in different inputs (e.g.dst.Relation=""from a parent edge,src.Relation="trades-with"from a related edge), the more informative relation is lost — and it's order-dependent on which tree was processed first.In practice the only path where the same node legitimately gets a non-empty
Relationis the related edge, andEdgeType="target"already dominates over"related"here, so the impact is narrow. Still, worth confirming this is intentional or deciding on an explicit precedence (e.g. prefer non-emptysrc.Relationwhendst.Relation == "").🤖 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 `@query/config_tree.go` around lines 81 - 98, mergeConfigTreeInto currently only updates EdgeType and can silently drop a non-empty Relation from src when dst.Relation is empty; change the merge logic in mergeConfigTreeInto so that when visiting the same node (function mergeConfigTreeInto) you also merge Relation with explicit precedence: if dst.Relation == "" and src.Relation != "" then set dst.Relation = src.Relation (but do not overwrite a non-empty dst.Relation), and ensure this applies both when promoting EdgeType to "target" and when appending cloned children (cloneConfigTree) so the preserved relation survives merges and is not lost due to processing order.
225-238: 💤 Low valueBehavior change worth flagging in commit/PR notes: silent skip of missing IDs.
getExistingConfigsByIDsswallowsgorm.ErrRecordNotFound, whereas the previously-usedGetConfigsByIDspropagated it (query/config.go:388-400). Call sites at lines 135 and 182 already cope with missing entries (the byID-map filtering inresolveParentsFromPath, and downstreambuildConfigTreeskipping unknown IDs), so functionally this is a deliberate relaxation. A debug-level log on skip would help diagnose stale-cache or cross-shard issues without spamming production logs, but it's optional for this PR.🤖 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 `@query/config_tree.go` around lines 225 - 238, getExistingConfigsByIDs silently skips missing IDs (errors.Is(err, gorm.ErrRecordNotFound)) which changes behavior; keep the same skip but emit a debug-level log when ConfigItemFromCache returns gorm.ErrRecordNotFound so stale-cache or cross-shard misses are visible without noise. Modify getExistingConfigsByIDs to, inside the errors.Is(err, gorm.ErrRecordNotFound) branch, call the project's debug logger (consistent with existing logging usage in this package) to log the missing id (id.String()) and brief context, then continue; leave other error handling and return values unchanged.connection/http.go (1)
355-364: ⚖️ Poor tradeoffConsider tightening
TransportWithContext'sctx anysignature.
ctx anyaccepts anything, includingniland arbitrary types, and pushes the type-narrowing down intoapplyHTTPObservability/harTokenTransport. This is fragile — a caller who passes the wrong concrete type silently loses observability with no compile-time signal. If onlycontext.ContextandConnectionContextare valid, two typed overloads (or a singlecontext.Contextparameter, withTransport()keeping the no-context shortcut) would be both safer and self-documenting.🤖 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 `@connection/http.go` around lines 355 - 364, Change TransportWithContext's first parameter from ctx any to a typed context (preferably ctx context.Context) to avoid accepting arbitrary types; update TransportWithContext(ctx context.Context, opts ...types.ClientOption) in the HTTPConnection receiver and adjust callers; then modify applyHTTPObservability and harTokenTransport to accept context.Context (or provide an adapter that extracts a ConnectionContext from context.Context) so observability and token transport get a well-typed context; alternatively, if you want two overloads, add TransportWithConnectionContext(connCtx ConnectionContext, opts...) in addition to Transport() so callers must pass either no-context or a strongly-typed context, and update httpConnectionRoundTripper construction in TransportWithContext to use the new typed APIs.Makefile (1)
94-101: 💤 Low valueVerify that
claudeCLI availability is handled.Unlike
gavelandcaptain, theclaudeCLI used at line 96 isn't auto-installed (and can't be viago install). Runningmake PROPERTIES.mdon a fresh checkout whereclaudeis missing will fail with a confusingcommand not found. Consider gating with a friendly check (e.g.,@command -v claude >/dev/null || { echo "claude CLI required: see https://docs.anthropic.com/..." >&2; exit 1; }) or documenting the prerequisite near the target.🤖 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 `@Makefile` around lines 94 - 101, The Makefile target PROPERTIES.md invokes the external claude CLI without checking availability, which causes a confusing "command not found" for fresh checkouts; update the PROPERTIES.md target (the rule that pipes the claude command into captain) to first check for the claude binary (e.g., use a shell check like command -v claude) and print a clear error and non-zero exit if missing, or gate the target with a phony prerequisite that validates claude presence and emits a helpful install/documentation message before attempting to run claude.connection/gke.go (1)
110-114: 💤 Low valueFunctionally safe but inconsistent with the
appendWrapTransportpattern.
restConfigis constructed locally at Lines 103-109 with noWrapTransportset, so directly assigning it here is correct. However, this duplicates the wrapping logic thatkubernetes.appendWrapTransportalready encapsulates (and that handles the compose-with-existing case). If that helper is exported (see comment onkubernetes/k8s.go), this block becomes a one-liner and the two call sites stay in lockstep.♻️ Use the shared helper (after exporting it)
- if middleware := httpObservabilityMiddleware(ctx, "kubernetes", o.HARCollector); middleware != nil { - restConfig.WrapTransport = func(rt http.RoundTripper) http.RoundTripper { - return middleware(rt) - } - } + dutyKubernetes.AppendWrapTransport(restConfig, httpObservabilityMiddleware(ctx, "kubernetes", o.HARCollector))🤖 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 `@connection/gke.go` around lines 110 - 114, The code duplicates transport-wrapping logic instead of using the shared helper: replace the manual WrapTransport assignment on restConfig (the block that calls httpObservabilityMiddleware with o.HARCollector) with a call to the shared appendWrapTransport helper (e.g., kubernetes.AppendWrapTransport(restConfig, httpObservabilityMiddleware(ctx, "kubernetes", o.HARCollector))) so it composes with any existing WrapTransport; ensure the helper is exported (AppendWrapTransport) if not already and pass the middleware result directly to it.kubernetes/k8s.go (2)
107-118: 💤 Low valueComposition is correct; consider exporting for reuse.
appendWrapTransportcorrectly chains atop any existingWrapTransport(including the one set bytrace).connection/gke.go(Lines 110-114) duplicates this pattern by directly assigningrestConfig.WrapTransport; exporting this helper (e.g.,AppendWrapTransport) and calling it there would centralize the composition logic and avoid the inconsistency where the GKE path overwrites instead of composes.🤖 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 `@kubernetes/k8s.go` around lines 107 - 118, The helper that composes rest.Config.WrapTransport should be exported and used by the GKE path: rename and export appendWrapTransport to AppendWrapTransport (preserving signature func AppendWrapTransport(config *rest.Config, middleware middlewares.Middleware)) and replace the direct assignment to restConfig.WrapTransport in the GKE connection code with a call to AppendWrapTransport so the new middleware composes with any existing WrapTransport (e.g., the one set by trace) instead of overwriting it; update any call sites to the new exported name.
160-171: 💤 Low valueSubtle: heuristic only triggers on a
"prefix; document the assumption.
normalizeKubeconfigOrPathdecodes JSON-encoded kubeconfig only when the trimmed input starts with". That works because YAML kubeconfigs realistically never start with a JSON string-quote, but it's an undocumented assumption. A short comment on the function explaining the discriminator — and noting that on any decode failure the original (untrimmed) value is returned to avoid silently stripping whitespace from a real path — would help future readers.🤖 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 `@kubernetes/k8s.go` around lines 160 - 171, Add a short comment above normalizeKubeconfigOrPath explaining the heuristic: it treats inputs that, after TrimSpace, start with a double-quote as JSON-string-encoded kubeconfigs (since real YAML kubeconfigs won't start with `"`), attempts json.Unmarshal into a string, and on any decode failure returns the original untrimmed value to avoid silently stripping whitespace from a real path; reference the function name normalizeKubeconfigOrPath and the variables trimmed/value in the comment so readers know why the discriminator and the fallback behavior are chosen.kubernetes/utils_test.go (1)
48-48: 💤 Low valuePassing
nillogger is fragile.
NewClientFromPathOrConfigWithMiddlewarehappens to never deref the logger on this test's code path (the input is decoded as kubeconfig content, so the path branch inNewClientWithMiddlewarethat callslog.Infofisn't hit). But any future change that adds a log call before/afternormalizeKubeconfigOrPathwould crash this test with an NPE. Preferlogger.GetLogger("test")(or the package's null-logger equivalent) to keep the test resilient.♻️ Use a real (silent) logger
- client, restConfig, err := NewClientFromPathOrConfigWithMiddleware(nil, string(encoded), nil) + client, restConfig, err := NewClientFromPathOrConfigWithMiddleware(logger.GetLogger("test"), string(encoded), nil)🤖 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 `@kubernetes/utils_test.go` at line 48, The test passes a nil logger into NewClientFromPathOrConfigWithMiddleware which is fragile; change the test to pass a real (silent) logger instead of nil (e.g., use logger.GetLogger("test") or the package's null-logger) when calling NewClientFromPathOrConfigWithMiddleware so future additions that call logging (e.g., in NewClientWithMiddleware or normalizeKubeconfigOrPath) won't cause a nil-pointer panic; update the client, restConfig, err := NewClientFromPathOrConfigWithMiddleware(...) invocation to supply that logger.views/038_config_access.sql (1)
16-38: 💤 Low valueActive-membership CTE correctly excludes soft-deleted memberships from grant fan-out.
Materialising
active_external_user_groupsas aDISTINCTnon-deleted view and using it in both the inner-join branch (line 36) and theNOT EXISTSguard (line 57-58) ensures soft-deleted memberships no longer produce per-user fan-out rows nor mask group-only rows.Minor stylistic nit (pre-existing, not introduced here):
config_access.deleted_at IS NULLandconfig_access.external_group_id IS NOT NULLbelong in aWHERErather than theONclause; their position in the join predicate is functionally fine for an INNER JOIN but reads awkwardly.🤖 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 `@views/038_config_access.sql` around lines 16 - 38, The JOIN currently places non-join predicates (config_access.deleted_at IS NULL and config_access.external_group_id IS NOT NULL) inside the ON clause of the INNER JOIN between config_access and the CTE active_external_user_groups; move those two predicates out of the ON clause and into a WHERE clause (keeping the INNER JOIN only for matching active_external_user_groups.external_group_id = config_access.external_group_id) so the intent is clearer and the join predicate only expresses the join key for config_access and active_external_user_groups.tests/config_access_test.go (1)
71-132: 💤 Low valueNo cleanup of test data inserted into a shared DB.
deletedGroup,deletedUser, anddeletedMembershipare inserted but never deleted; they will persist for the rest of the suite run. While the random UUIDs avoid collisions with other tests, this leaks rows intoexternal_groups/external_users/external_user_groupsthat subsequent suite-wide queries (e.g., other counts in the sameOrderedblock) could be sensitive to. Consider anAfterEach/DeferCleanupto remove them.🤖 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 `@tests/config_access_test.go` around lines 71 - 132, Test inserts deletedGroup, deletedUser and deletedMembership but never removes them; add cleanup to delete those rows after the spec runs (use Ginkgo's DeferCleanup or AfterEach) by invoking DefaultContext.DB().Delete on models.ExternalGroup, models.ExternalUser and models.ExternalUserGroup using the inserted IDs (deletedGroup.ID, deletedUser.ID and the composite keys from deletedMembership) so the temporary rows are removed and do not leak into subsequent tests.tests/config_relationship_test.go (1)
440-502: 💤 Low valueConsider using an existing scraper fixture to future-proof the test.
The
config_relationships.scraper_idcolumn currently has no foreign key constraint, so the test works fine withuuid.New(). However, if a FK is added later,ownedRelinsertion could fail. SwappingscraperIDto reference an existing fixture (e.g., from the dummy data) would make the test more resilient to future schema changes with minimal effort.🤖 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 `@tests/config_relationship_test.go` around lines 440 - 502, The test creates a random scraperID (scraperID) and assigns it to ownedRel, which could break if a foreign key is later added; change the initializer in the BeforeAll so scraperID (and ownedRel.ScraperID) uses an existing dummy fixture scraper UUID (e.g., dummy.Scraper.ID or the appropriate existing dummy scraper field) instead of uuid.New(), keeping the same scraperID variable and ownedRel construction so the rest of the test and UpdateIsPushed call remain unchanged.
🤖 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 `@connection/git_logging_test.go`:
- Around line 98-138: The helper makeBareRepo invokes the git executable without
checking availability, causing test failures on systems without Git; modify
makeBareRepo to call exec.LookPath("git") at the start and call t.Skipf(...) if
git is not found so the test is skipped gracefully, and apply the same
availability check/skip to any other test helpers in this file that shell out to
git (the other helper used around the later block that also runs
exec.Command("git", ...)) so tests do not fail on minimal CI images.
In `@connection/git.go`:
- Line 29: The global mutex gitHTTPTransportMu and the configureGitHTTPTransport
function currently install wrapped http/https protocols via
gitClient.InstallProtocol during each GitClient.Clone and hold the lock for the
entire clone, which serializes all clones process-wide; change this by (1)
adding an explanatory comment near gitHTTPTransportMu and
configureGitHTTPTransport/GitClient.Clone describing this serialization and
visibility to all go-git users, and (2) refactor to install the wrapped protocol
clients once at startup (when HTTP observability/logging is enabled) instead of
per-clone — or alternatively implement and install a single lightweight
RoundTripper that dispatches behavior based on http.Request.Context() so
per-call context is respected without re-registering protocols; ensure
references to gitClient.InstallProtocol, configureGitHTTPTransport,
restoreGitTransport, and GitClient.Clone are updated to reflect the new
startup/one-time registration approach and remove per-clone lock usage.
In `@context/template.go`:
- Around line 18-25: The current logging branches in context/template.go leak
secrets by printing the full env and rendered template (see l.V(3).Infof using
logger.Pretty(env) and the high-verbosity rendered output); replace these prints
with non-sensitive summaries: log only env keys or a count (use lo.Keys(env) or
len(env)) and never log the full rendered template/value. Update the
l.V(3).Infof and the analogous branch at lines 58-65 to call a new helper like
sanitizeEnvKeysOrMask(env) (or simply use lo.Keys(env)/len(env)) and remove any
logger.Pretty(env)/rendered value outputs so secrets are not emitted.
In `@Makefile`:
- Around line 18-35: Add the targets "test" and "test-concurrent" to the
Makefile's .PHONY declarations so Make always treats them as phony targets;
update the existing .PHONY line (or add a new .PHONY line) to include test and
test-concurrent alongside the other phony targets to match the surrounding
targets like test-e2e and test-e2e-blobs.
In `@PROPERTIES.md`:
- Around line 9-10: The README contains an absolute, developer-local link to
PROPERTIES.schema.json; update the link target to a relative path (e.g.,
./PROPERTIES.schema.json or PROPERTIES.schema.json) so the reference in
PROPERTIES.md points to the repository file rather than /Users/moshe/…; edit the
line that currently links to
/Users/moshe/go/src/github.com/flanksource/duty/PROPERTIES.schema.json and
replace it with the relative path to PROPERTIES.schema.json.
In `@query/resource_selector.go`:
- Around line 272-279: searchSetID only matches the literal "id", missing
table-specific ids like "config_id" or "component_id"; update the searchSetID
computation (the slice ContainsFunc over flatFields that uses qm.Aliases) to
consider both canonical alias mapping and table-specific id patterns—e.g., after
resolving alias (field := strings.ToLower(s); if alias, field = alias), treat
the field as matching if field == "id" OR strings.HasSuffix(field, "_id") (or
match a regex like ^[a-z]+_id$), so queries explicitly on config_id/component_id
will set searchSetID=true and avoid the auto-agent filter being applied.
In `@schema/config_access.hcl`:
- Around line 127-131: The schema currently sets a steady-state default
"00000000-0000-0000-0000-000000000000" for column "scraper_id" on table
external_user_groups which can hide missed propagation bugs; remove the default
from the production HCL declaration so the column has no default, and instead
ensure any one-off migration/backfill step sets that nil-UUID only during
backfill (e.g., in the migration script that updates external_user_groups) and
then drops/does not persist the default in the steady-state schema (update the
column "scraper_id" block to remove the default and keep null=false/type=uuid).
In `@tests/config_access_test.go`:
- Around line 117-131: The test silently passes when rows are missing because
byID[...] yields a zero-value externalGroupSummary; ensure presence before
asserting counts by first asserting len(summaries) (or expected number of rows)
and then checking map membership for each expected ID (e.g.
dummy.MissionControlAdminsGroup.ID, dummy.MissionControlReadersGroup.ID,
dummy.MissionControlEmptyGroup.ID, deletedGroup.ID) using a presence check (e.g.
_, ok := byID[id] / Expect(ok).To(BeTrue()) or HaveKey) before asserting
MembersCount and PermissionsCount on the retrieved externalGroupSummary values.
In `@tests/setup/common.go`:
- Around line 186-188: The current code replaces "/postgres" by string
manipulation which can corrupt credentials or query params; instead parse the
DSN with net/url (or the appropriate driver DSN parser), set the path to the new
database name (dbName) and re-encode it before assigning PgUrl; update the logic
around postgresDBUrl, dbName and PgUrl so you construct PgUrl by parsing
postgresDBUrl, replacing only the URL.Path (or the DSN's database field) with
"/"+dbName, and serializing back to a string.
---
Outside diff comments:
In `@connection/http.go`:
- Around line 351-410: TransportWithContext currently constructs base as
&netHTTP.Transport{} via applyHTTPObservability without applying
HTTPConnection.TLS, so TLS settings are ignored and the later rt.TLS = conn.TLS
in httpConnectionRoundTripper.RoundTrip is a no-op; update TransportWithContext
(the Transport and TransportWithContext functions) to take conn := h (or access
h.TLS) and apply conn.TLS into the underlying netHTTP.Transport (CA, Cert/Key,
InsecureSkipVerify, HandshakeTimeout etc.) before calling
applyHTTPObservability/creating base, mirroring how
CreateHTTPClient/client.TLSConfig applies TLS, and remove the dead assignment in
httpConnectionRoundTripper.RoundTrip that sets rt.TLS = conn.TLS.
In `@query/resource_selector.go`:
- Around line 293-322: The agentID declared at the top of the block is being
shadowed by the inner short-declaration (agentID, err := getAgentID(...)) inside
the qm.HasAgents branch, so the outer agentID remains nil and the subsequent
getScopeID(ctx, resourceSelector.Scope, table, agentID) call is unscoped; change
the inner declaration to assign to the outer variable (use agentID, err =
getAgentID(...)) so the resolved agent is passed into getScopeID, and ensure
error handling remains the same; check the qm.HasAgents, searchSetAgent, and
searchSetID logic around resourceSelector.Scope and table
(checks/config_items/components/config_changes/catalog_changes) to confirm
scoping behavior is preserved.
In `@tests/setup/common.go`:
- Around line 278-299: The DB creation/drop SQL interpolates caller-supplied
newName directly (seen in the ctx.DB().Exec("CREATE DATABASE %s", newName) and
DROP DATABASE ... (FORCE) calls), reintroducing SQL-injection risk; instead
either validate newName against a strict identifier regex (e.g.
^[A-Za-z_][A-Za-z0-9_]*$) before use or quote it safely with a proper
identifier-quoting helper (e.g. pq.QuoteIdentifier or your project's equivalent)
and use that quoted value in the Exec calls; update the code paths around
newName, the CREATE DATABASE and DROP DATABASE Exec invocations, to use the
validated or quoted identifier.
---
Nitpick comments:
In `@connection/gke.go`:
- Around line 110-114: The code duplicates transport-wrapping logic instead of
using the shared helper: replace the manual WrapTransport assignment on
restConfig (the block that calls httpObservabilityMiddleware with
o.HARCollector) with a call to the shared appendWrapTransport helper (e.g.,
kubernetes.AppendWrapTransport(restConfig, httpObservabilityMiddleware(ctx,
"kubernetes", o.HARCollector))) so it composes with any existing WrapTransport;
ensure the helper is exported (AppendWrapTransport) if not already and pass the
middleware result directly to it.
In `@connection/http.go`:
- Around line 355-364: Change TransportWithContext's first parameter from ctx
any to a typed context (preferably ctx context.Context) to avoid accepting
arbitrary types; update TransportWithContext(ctx context.Context, opts
...types.ClientOption) in the HTTPConnection receiver and adjust callers; then
modify applyHTTPObservability and harTokenTransport to accept context.Context
(or provide an adapter that extracts a ConnectionContext from context.Context)
so observability and token transport get a well-typed context; alternatively, if
you want two overloads, add TransportWithConnectionContext(connCtx
ConnectionContext, opts...) in addition to Transport() so callers must pass
either no-context or a strongly-typed context, and update
httpConnectionRoundTripper construction in TransportWithContext to use the new
typed APIs.
In `@kubernetes/k8s.go`:
- Around line 107-118: The helper that composes rest.Config.WrapTransport should
be exported and used by the GKE path: rename and export appendWrapTransport to
AppendWrapTransport (preserving signature func AppendWrapTransport(config
*rest.Config, middleware middlewares.Middleware)) and replace the direct
assignment to restConfig.WrapTransport in the GKE connection code with a call to
AppendWrapTransport so the new middleware composes with any existing
WrapTransport (e.g., the one set by trace) instead of overwriting it; update any
call sites to the new exported name.
- Around line 160-171: Add a short comment above normalizeKubeconfigOrPath
explaining the heuristic: it treats inputs that, after TrimSpace, start with a
double-quote as JSON-string-encoded kubeconfigs (since real YAML kubeconfigs
won't start with `"`), attempts json.Unmarshal into a string, and on any decode
failure returns the original untrimmed value to avoid silently stripping
whitespace from a real path; reference the function name
normalizeKubeconfigOrPath and the variables trimmed/value in the comment so
readers know why the discriminator and the fallback behavior are chosen.
In `@kubernetes/utils_test.go`:
- Line 48: The test passes a nil logger into
NewClientFromPathOrConfigWithMiddleware which is fragile; change the test to
pass a real (silent) logger instead of nil (e.g., use logger.GetLogger("test")
or the package's null-logger) when calling
NewClientFromPathOrConfigWithMiddleware so future additions that call logging
(e.g., in NewClientWithMiddleware or normalizeKubeconfigOrPath) won't cause a
nil-pointer panic; update the client, restConfig, err :=
NewClientFromPathOrConfigWithMiddleware(...) invocation to supply that logger.
In `@Makefile`:
- Around line 94-101: The Makefile target PROPERTIES.md invokes the external
claude CLI without checking availability, which causes a confusing "command not
found" for fresh checkouts; update the PROPERTIES.md target (the rule that pipes
the claude command into captain) to first check for the claude binary (e.g., use
a shell check like command -v claude) and print a clear error and non-zero exit
if missing, or gate the target with a phony prerequisite that validates claude
presence and emits a helpful install/documentation message before attempting to
run claude.
In `@models/common.go`:
- Around line 91-95: The HealthUnknown case in the badge switch duplicates the
default branch in the function that returns api.Badge (located in
models/common.go), so remove the explicit "case HealthUnknown:" branch to rely
on the default branch, or if you want a distinct presentation keep the case but
change its styling to differ from default (e.g., use the same styling as
Pretty() or a unique bg/text class) and update the api.Badge call in that branch
accordingly.
In `@query/config_tree.go`:
- Around line 35-98: Update the MergeConfigTrees docstring to explicitly state
that the returned "forest" can be a DAG with shared *ConfigTreeNode pointers
across roots (because cloneConfigTree returns existing nodes from byID), so
callers should not assume disjoint trees; note that mutations to a shared node
will affect all roots that reference it, JSON serializers may duplicate the
shared subtree under each parent, and walkers should use a visited set to avoid
repeated processing (refer to MergeConfigTrees, cloneConfigTree, and the
behavior exercised by TestMergeConfigTreesSharedInternalNode).
- Around line 81-98: mergeConfigTreeInto currently only updates EdgeType and can
silently drop a non-empty Relation from src when dst.Relation is empty; change
the merge logic in mergeConfigTreeInto so that when visiting the same node
(function mergeConfigTreeInto) you also merge Relation with explicit precedence:
if dst.Relation == "" and src.Relation != "" then set dst.Relation =
src.Relation (but do not overwrite a non-empty dst.Relation), and ensure this
applies both when promoting EdgeType to "target" and when appending cloned
children (cloneConfigTree) so the preserved relation survives merges and is not
lost due to processing order.
- Around line 225-238: getExistingConfigsByIDs silently skips missing IDs
(errors.Is(err, gorm.ErrRecordNotFound)) which changes behavior; keep the same
skip but emit a debug-level log when ConfigItemFromCache returns
gorm.ErrRecordNotFound so stale-cache or cross-shard misses are visible without
noise. Modify getExistingConfigsByIDs to, inside the errors.Is(err,
gorm.ErrRecordNotFound) branch, call the project's debug logger (consistent with
existing logging usage in this package) to log the missing id (id.String()) and
brief context, then continue; leave other error handling and return values
unchanged.
In `@tests/config_access_test.go`:
- Around line 71-132: Test inserts deletedGroup, deletedUser and
deletedMembership but never removes them; add cleanup to delete those rows after
the spec runs (use Ginkgo's DeferCleanup or AfterEach) by invoking
DefaultContext.DB().Delete on models.ExternalGroup, models.ExternalUser and
models.ExternalUserGroup using the inserted IDs (deletedGroup.ID, deletedUser.ID
and the composite keys from deletedMembership) so the temporary rows are removed
and do not leak into subsequent tests.
In `@tests/config_relationship_test.go`:
- Around line 440-502: The test creates a random scraperID (scraperID) and
assigns it to ownedRel, which could break if a foreign key is later added;
change the initializer in the BeforeAll so scraperID (and ownedRel.ScraperID)
uses an existing dummy fixture scraper UUID (e.g., dummy.Scraper.ID or the
appropriate existing dummy scraper field) instead of uuid.New(), keeping the
same scraperID variable and ownedRel construction so the rest of the test and
UpdateIsPushed call remain unchanged.
In `@views/038_config_access.sql`:
- Around line 16-38: The JOIN currently places non-join predicates
(config_access.deleted_at IS NULL and config_access.external_group_id IS NOT
NULL) inside the ON clause of the INNER JOIN between config_access and the CTE
active_external_user_groups; move those two predicates out of the ON clause and
into a WHERE clause (keeping the INNER JOIN only for matching
active_external_user_groups.external_group_id = config_access.external_group_id)
so the intent is clearer and the join predicate only expresses the join key for
config_access and active_external_user_groups.
🪄 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
Run ID: 95dcc6e2-e5c0-40f5-9d41-d2ea45e3c396
📒 Files selected for processing (44)
.gitignoreMakefilePROPERTIES.mdPROPERTIES.schema.jsonTaskfile.yamlconnection/aws.goconnection/cnrm.goconnection/common.goconnection/eks.goconnection/gcs.goconnection/git.goconnection/git_logging_test.goconnection/gke.goconnection/http.goconnection/kubernetes.goconnection/observability_test.goconnection/opensearch.goconnection/prometheus.gocontext/properties.gocontext/template.gokubernetes/k8s.gokubernetes/utils_test.gomodels/common.gomodels/config.gomodels/config_access.goquery/config_tree.goquery/config_tree_test.goquery/resource_selector.gorbac/objects.goschema/config.hclschema/config_access.hclstart.gostart_test.gosuite_test.gotests/config_access_test.gotests/config_relationship_test.gotests/e2e-blobs/containers.gotests/fixtures/dummy/all.gotests/setup/common.gotypes/client_options.goviews/006_config_views.sqlviews/038_config_access.sqlviews/045_merge_external_entities.sqlviews/9998_rls_enable.sql
Comment on lines
+98
to
+138
| func TestGitCloneTraceLogging(t *testing.T) { | ||
| g := gomega.NewWithT(t) | ||
|
|
||
| root := t.TempDir() | ||
| remote := makeBareRepo(t, root) | ||
|
|
||
| logger.UseSlog() | ||
|
|
||
| readOut, stop := captureLoggerOutput(t) | ||
| defer stop() | ||
|
|
||
| setGitLogLevel(t, "trace") | ||
|
|
||
| dst := filepath.Join(root, "checkout") | ||
| gitClient := &GitClient{URL: remote, Branch: "main", Depth: 1} | ||
|
|
||
| ctx := context.NewContext(gocontext.TODO()) | ||
| _, err := gitClient.Clone(ctx, dst) | ||
| g.Expect(err).ToNot(gomega.HaveOccurred()) | ||
|
|
||
| // Simulate a transport progress line going through the same writer | ||
| // Clone hands to go-git, to prove the path is live. A real network | ||
| // clone emits lines like this via sideband; local/file clones do not. | ||
| fmt.Fprintln(logger.GetLogger("git").V(2), "Counting objects: 3, done.") | ||
|
|
||
| stop() | ||
| out := readOut() | ||
|
|
||
| want := []string{ | ||
| "clone url=", | ||
| "branch=main", | ||
| "depth=1", | ||
| "dir=" + dst, | ||
| "checked out ", | ||
| "Counting objects: 3, done.", | ||
| "(git)", | ||
| } | ||
| for _, w := range want { | ||
| g.Expect(out).To(gomega.ContainSubstring(w), "trace output missing %q", w) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether commons logger exposes a way to save/restore the active backend.
rg -nP -C3 '\bUseSlog\b|\bSetLogger\b|\bGetLogger\b\s*\(\s*\)' --type=goRepository: flanksource/duty
Length of output: 2522
🏁 Script executed:
#!/bin/bash
# 1. Find the logger package to check available functions
fd -e '.go' | xargs grep -l 'package logger' | head -5Repository: flanksource/duty
Length of output: 42
🏁 Script executed:
#!/bin/bash
# 2. Check makeBareRepo implementation
sed -n '1,/^func makeBareRepo/p' connection/git_logging_test.go | tail -30Repository: flanksource/duty
Length of output: 540
🏁 Script executed:
#!/bin/bash
# 3. Read makeBareRepo full function
rg -A 30 'func makeBareRepo\(' connection/git_logging_test.goRepository: flanksource/duty
Length of output: 1162
🏁 Script executed:
#!/bin/bash
# 4. Check if there's a way to restore logger in commons
rg -nP 'GetOutput|SetOutput|RestoreLogger|SaveLogger' --type=go | head -20Repository: flanksource/duty
Length of output: 500
🏁 Script executed:
#!/bin/bash
# Find logger package definition
find . -name "*.go" -path "*/commons/*" | xargs grep -l 'func UseSlog' | head -3Repository: flanksource/duty
Length of output: 42
🏁 Script executed:
#!/bin/bash
# If commons is external, check in vendor or go.mod
rg 'github\.com/flanksource/commons' go.mod | head -5Repository: flanksource/duty
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Search for any logger implementation getter/setter for the backend itself
rg -nP '\bGetLogger\b\s*\(\s*\)|\bSetLogger\b\s*\(\s*logger' --type=go | head -15Repository: flanksource/duty
Length of output: 301
🏁 Script executed:
#!/bin/bash
# Check the complete git_logging_test.go to see all test functions and cleanup patterns
wc -l connection/git_logging_test.go && sed -n '1,50p' connection/git_logging_test.goRepository: flanksource/duty
Length of output: 1544
🏁 Script executed:
#!/bin/bash
# Read the second test function mentioned (lines 143-174)
sed -n '143,174p' connection/git_logging_test.goRepository: flanksource/duty
Length of output: 1001
🏁 Script executed:
#!/bin/bash
# Check for any cleanup/defer patterns with logger in the file
rg -n 'defer|t\.Cleanup|t\.TempDir' connection/git_logging_test.goRepository: flanksource/duty
Length of output: 256
🏁 Script executed:
#!/bin/bash
# Search for other test files that call UseSlog to see if they restore it
rg -l 'UseSlog' --type=go | xargs -I {} sh -c 'echo "=== {} ===" && grep -A 5 -B 2 "UseSlog" {}'Repository: flanksource/duty
Length of output: 626
🏁 Script executed:
#!/bin/bash
# Check commons logger package documentation/API in go.mod or vendor
find . -path "*vendor/github.com/flanksource/commons/logger*" -name "*.go" 2>/dev/null | head -3Repository: flanksource/duty
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Read captureLoggerOutput to understand the complete logger setup/teardown
sed -n '46,90p' connection/git_logging_test.goRepository: flanksource/duty
Length of output: 1322
🏁 Script executed:
#!/bin/bash
# Check if there are other test files that might depend on non-slog logger behavior
find . -name '*_test.go' -type f | xargs grep -l 'logger\.' | wc -lRepository: flanksource/duty
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Verify if UseSlog is truly idempotent by checking if multiple calls cause issues
rg -B 5 -A 10 'func UseSlog' --type=go | head -20Repository: flanksource/duty
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if there's a GetLogger() or similar that returns the current implementation
rg -nP 'func.*Get.*ogger\b.*\(\)' --type=goRepository: flanksource/duty
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if commons has this function by looking for any pattern that might help
rg 'SetLogger|GetLogger|RestoreLogger' --type=go -B 2 -A 2 | grep -E '(func|logger)' | head -20Repository: flanksource/duty
Length of output: 1599
🏁 Script executed:
#!/bin/bash
# Verify the complete setGitLogLevel function to confirm cleanup pattern
sed -n '87,95p' connection/git_logging_test.goRepository: flanksource/duty
Length of output: 413
🏁 Script executed:
#!/bin/bash
# Check if UseSlog() is called elsewhere and if tests following it depend on non-slog behavior
rg -B 3 -A 8 'func Test' connection/git_logging_test.go | grep -E '(func Test|UseSlog|Cleanup)'Repository: flanksource/duty
Length of output: 195
Add git availability check to makeBareRepo to skip gracefully when unavailable.
The test helper shells out to git via exec.Command without checking if the binary exists. On environments without git installed (minimal CI runners, etc.), the test fails rather than skips. Use exec.LookPath("git") and t.Skip(...) to degrade gracefully.
♻️ Skip when git is unavailable
func makeBareRepo(t *testing.T, root string) string {
t.Helper()
g := gomega.NewWithT(t)
+ if _, err := exec.LookPath("git"); err != nil {
+ t.Skipf("git binary not available: %v", err)
+ }
work := filepath.Join(root, "work")Also applies to: 143-174
🤖 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 `@connection/git_logging_test.go` around lines 98 - 138, The helper
makeBareRepo invokes the git executable without checking availability, causing
test failures on systems without Git; modify makeBareRepo to call
exec.LookPath("git") at the start and call t.Skipf(...) if git is not found so
the test is skipped gracefully, and apply the same availability check/skip to
any other test helpers in this file that shell out to git (the other helper used
around the later block that also runs exec.Command("git", ...)) so tests do not
fail on minimal CI images.
| "github.com/flanksource/duty/types" | ||
| ) | ||
|
|
||
| var gitHTTPTransportMu sync.Mutex |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate any other callers of go-git's http/transport client registration that could collide.
rg -nP -C3 'plumbing/transport/client' --type=go
rg -nP -C3 'InstallProtocol\s*\(' --type=go
# Find other Clone/Fetch call sites that could run concurrently.
rg -nP -C3 '\bgit\.PlainClone(Context)?\s*\(|\bFetchContext\s*\(' --type=goRepository: flanksource/duty
Length of output: 1942
🏁 Script executed:
#!/bin/bash
# Read the Clone function to see where configureGitHTTPTransport is called and deferred
sed -n '80,200p' connection/git.go | cat -nRepository: flanksource/duty
Length of output: 4939
🏁 Script executed:
#!/bin/bash
# Get the complete configureGitHTTPTransport function and surrounding context
sed -n '375,410p' connection/git.go | cat -nRepository: flanksource/duty
Length of output: 949
🏁 Script executed:
#!/bin/bash
# Search for all Clone method definitions and FetchContext usage in this file
rg -n 'func.*Clone|defer.*configureGitHTTPTransport|FetchContext' connection/git.goRepository: flanksource/duty
Length of output: 214
🏁 Script executed:
#!/bin/bash
# Search for other callers of GitClient.Clone or direct go-git usage
rg -n 'GitClient.*Clone|git\.PlainClone|go-git' --type=go -i | head -30Repository: flanksource/duty
Length of output: 1348
🏁 Script executed:
#!/bin/bash
# Search for goroutine patterns that might call Clone
rg -n 'go\s+.*Clone|Clone.*goroutine' --type=go -iRepository: flanksource/duty
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if InstallProtocol is called anywhere else
rg -rn 'InstallProtocol' --type=goRepository: flanksource/duty
Length of output: 282
🏁 Script executed:
#!/bin/bash
# Find all callers of .Clone()
rg -n '\.Clone\(' --type=go | grep -v test | grep -v 'func.*Clone'Repository: flanksource/duty
Length of output: 1483
🏁 Script executed:
#!/bin/bash
# Search for direct uses of git package operations outside of GitClient
rg -n 'git\.Plain|git\.Open' --type=go | grep -v test | grep -v connection/git.goRepository: flanksource/duty
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look at the Clone call in shell.go and surrounding context
sed -n '310,330p' shell/shell.go | cat -nRepository: flanksource/duty
Length of output: 662
🏁 Script executed:
#!/bin/bash
# Search for callers of the function containing the Clone call
rg -B20 'client\.Clone\(ctx, result\.mountPoint\)' shell/shell.go | head -40Repository: flanksource/duty
Length of output: 849
Global mutex serializes concurrent git operations process-wide when HTTP observability is enabled.
configureGitHTTPTransport mutates go-git's package-global protocol registry via gitClient.InstallProtocol (lines 391–392), and gitHTTPTransportMu is held from Lock (line 389) until the deferred cleanup runs (line 26 in the Clone function, via defer restoreGitTransport()). Consequently:
- When HAR/logging is enabled for
"git", everyClonecall across all goroutines is fully serialized on this mutex for the duration of the clone (potentially many seconds for large repos). - The mutated
http/httpsprotocol clients are visible to any go-git user in the same process for the lock window — not just calls flowing throughGitClient.Clone— which is broader than intended.
The per-mountpoint lock in shell/shell.go (line 320) protects individual checkout operations, but the global mutex still serializes all concurrent Clone operations across different repositories.
go-git does not expose a per-CloneOptions HTTP client, so the global registration is a known constraint. Consider mitigations:
- Document this serialization behavior explicitly in code comments so future maintainers understand the performance cost.
- Install the wrapped clients once at startup (when observability is configured) rather than per-clone, eliminating per-operation lock contention. The middleware closure can read per-call context via
http.Request.Context(). - If per-call context varies by operation, use a lightweight
RoundTripperthat dispatches based on a context value, installed once globally.
🤖 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 `@connection/git.go` at line 29, The global mutex gitHTTPTransportMu and the
configureGitHTTPTransport function currently install wrapped http/https
protocols via gitClient.InstallProtocol during each GitClient.Clone and hold the
lock for the entire clone, which serializes all clones process-wide; change this
by (1) adding an explanatory comment near gitHTTPTransportMu and
configureGitHTTPTransport/GitClient.Clone describing this serialization and
visibility to all go-git users, and (2) refactor to install the wrapped protocol
clients once at startup (when HTTP observability/logging is enabled) instead of
per-clone — or alternatively implement and install a single lightweight
RoundTripper that dispatches behavior based on http.Request.Context() so
per-call context is respected without re-registering protocols; ensure
references to gitClient.InstallProtocol, configureGitHTTPTransport,
restoreGitTransport, and GitClient.Clone are updated to reflect the new
startup/one-time registration approach and remove per-clone lock usage.
Comment on lines
+18
to
+25
| l := k.Logger.Named("template") | ||
| if l.V(3).Enabled() { | ||
| l.V(3).Infof("Running template: %s with environment: %v", t.String(), logger.Pretty(env)) | ||
| } else if l.IsLevelEnabled(logger.Trace) { | ||
| l.V(2).Infof("Running template: %s with environment keys: %v", t.String(), lo.Keys(env)) | ||
| } else { | ||
| l.V(1).Infof("Running template: %s", t.String()) | ||
| } |
Contributor
There was a problem hiding this comment.
Don't dump raw template inputs and outputs to the logger.
These new branches log the full env map and the fully rendered value. Template environments regularly carry secrets, so turning verbosity up will now spill credentials and tokens into logs.
🔒 Suggested direction
- if l.V(3).Enabled() {
- l.V(3).Infof("Running template: %s with environment: %v", t.String(), logger.Pretty(env))
- } else if l.IsLevelEnabled(logger.Trace) {
+ if l.IsLevelEnabled(logger.Trace) {
l.V(2).Infof("Running template: %s with environment keys: %v", t.String(), lo.Keys(env))
} else {
l.V(1).Infof("Running template: %s", t.String())
}
@@
- } else if t.Template != val {
- if l.V(2).Enabled() {
- l.V(2).Infof("%s = %s", t.String(), val)
- } else if l.V(1).Enabled() {
- l.V(1).Infof("templated %s = changed", t.String())
- }
+ } else if t.Template != val {
+ l.V(1).Infof("templated %s = changed", t.String())
}Also applies to: 58-65
🤖 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 `@context/template.go` around lines 18 - 25, The current logging branches in
context/template.go leak secrets by printing the full env and rendered template
(see l.V(3).Infof using logger.Pretty(env) and the high-verbosity rendered
output); replace these prints with non-sensitive summaries: log only env keys or
a count (use lo.Keys(env) or len(env)) and never log the full rendered
template/value. Update the l.V(3).Infof and the analogous branch at lines 58-65
to call a new helper like sanitizeEnvKeysOrMask(env) (or simply use
lo.Keys(env)/len(env)) and remove any logger.Pretty(env)/rendered value outputs
so secrets are not emitted.
Comment on lines
+272
to
+279
| searchSetID = slices.ContainsFunc(flatFields, func(s string) bool { | ||
| field := strings.ToLower(s) | ||
| if alias, ok := qm.Aliases[field]; ok { | ||
| field = alias | ||
| } | ||
| return field == "id" | ||
| }) | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect Aliases definitions for the query models to confirm id-related aliases resolve to "id".
rg -nP --type=go -C5 '\bAliases\s*:\s*map\[string\]string'
ast-grep --pattern 'Aliases: map[string]string{ $$$ }'Repository: flanksource/duty
Length of output: 4042
🏁 Script executed:
#!/bin/bash
# Check how searchSetID is used and what its impact is
rg -n 'searchSetID' query/resource_selector.go -B2 -A2
# Also check if there are any references to config_id, component_id as primary identifiers
rg -n 'config_id|component_id' query/resource_selector.go -C2Repository: flanksource/duty
Length of output: 1031
searchSetID only recognizes literal "id" field, but tables use specific id variants like config_id and component_id without alias mappings.
The id detection currently checks only for the literal string "id", but the codebase uses table-specific identifiers (e.g., config_id for config-related tables, component_id for components). None of the QueryModel alias maps contain entries mapping these back to "id". This means queries using config_id or component_id will not set searchSetID=true and the auto-agent filter at line 294 will still apply even when users explicitly query by id—a silent correctness bug.
Either:
- Add alias entries (
"config_id": "id","component_id": "id", etc.) to the respective QueryModels, or - Expand the
searchSetIDcheck to recognize table-specific id field patterns.
🤖 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 `@query/resource_selector.go` around lines 272 - 279, searchSetID only matches
the literal "id", missing table-specific ids like "config_id" or "component_id";
update the searchSetID computation (the slice ContainsFunc over flatFields that
uses qm.Aliases) to consider both canonical alias mapping and table-specific id
patterns—e.g., after resolving alias (field := strings.ToLower(s); if alias,
field = alias), treat the field as matching if field == "id" OR
strings.HasSuffix(field, "_id") (or match a regex like ^[a-z]+_id$), so queries
explicitly on config_id/component_id will set searchSetID=true and avoid the
auto-agent filter being applied.
Comment on lines
+127
to
+131
| column "scraper_id" { | ||
| type = uuid | ||
| null = false | ||
| default = "00000000-0000-0000-0000-000000000000" | ||
| } |
Contributor
There was a problem hiding this comment.
Drop the steady-state default for external_user_groups.scraper_id after backfill.
Now that scraper_id is part of the primary key, leaving the nil-UUID default here turns missed propagation bugs into silent legacy rows instead of failing fast. That makes it easy to split memberships across 00000000-0000-0000-0000-000000000000 and the real scraper owner.
🧭 Suggested direction
column "scraper_id" {
type = uuid
null = false
- default = "00000000-0000-0000-0000-000000000000"
}If the default is only needed to migrate existing rows, keep it in the migration/backfill step and drop it from the steady-state schema.
📝 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
| column "scraper_id" { | |
| type = uuid | |
| null = false | |
| default = "00000000-0000-0000-0000-000000000000" | |
| } | |
| column "scraper_id" { | |
| type = uuid | |
| null = false | |
| } |
🤖 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 `@schema/config_access.hcl` around lines 127 - 131, The schema currently sets a
steady-state default "00000000-0000-0000-0000-000000000000" for column
"scraper_id" on table external_user_groups which can hide missed propagation
bugs; remove the default from the production HCL declaration so the column has
no default, and instead ensure any one-off migration/backfill step sets that
nil-UUID only during backfill (e.g., in the migration script that updates
external_user_groups) and then drops/does not persist the default in the
steady-state schema (update the column "scraper_id" block to remove the default
and keep null=false/type=uuid).
Comment on lines
186
to
+188
| postgresDBUrl = url | ||
| dbName = randomDatabaseName("duty_test") | ||
|
|
||
| var err error | ||
| PgUrl, err = databaseURL(url, dbName) | ||
| if err != nil { | ||
| return context.Context{}, err | ||
| } | ||
|
|
||
| quotedDBName := quoteIdentifier(dbName) | ||
| _ = execPostgres(postgresDBUrl, fmt.Sprintf("DROP DATABASE IF EXISTS %s (FORCE)", quotedDBName)) | ||
| if err := execPostgres(postgresDBUrl, fmt.Sprintf("CREATE DATABASE %s", quotedDBName)); err != nil { | ||
| dbName = fmt.Sprintf("duty_gingko%d", port) | ||
| PgUrl = strings.Replace(url, "/postgres", "/"+dbName, 1) |
Contributor
There was a problem hiding this comment.
Parse the DSN and replace only the database path.
Both of these rewrites operate on the full URL string. That will fail whenever DUTY_DB_URL is not pointing at /postgres, and ReplaceAll can also rewrite matching substrings in credentials, query params, or the host.
🛠️ Suggested fix
+func withDatabase(rawURL, dbName string) (string, error) {
+ u, err := url.Parse(rawURL)
+ if err != nil {
+ return "", err
+ }
+ u.Path = "/" + dbName
+ return u.String(), nil
+}
+
postgresDBUrl = url
dbName = fmt.Sprintf("duty_gingko%d", port)
- PgUrl = strings.Replace(url, "/postgres", "/"+dbName, 1)
+ PgUrl, err = withDatabase(url, dbName)
+ if err != nil {
+ return context.Context{}, err
+ }
@@
- config := api.NewConfig(strings.ReplaceAll(pgUrl, pgDbName, newName))
+ nextURL, err := withDatabase(pgUrl, newName)
+ if err != nil {
+ return nil, nil, err
+ }
+ config := api.NewConfig(nextURL)Also applies to: 282-282
🤖 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 `@tests/setup/common.go` around lines 186 - 188, The current code replaces
"/postgres" by string manipulation which can corrupt credentials or query
params; instead parse the DSN with net/url (or the appropriate driver DSN
parser), set the path to the new database name (dbName) and re-encode it before
assigning PgUrl; update the logic around postgresDBUrl, dbName and PgUrl so you
construct PgUrl by parsing postgresDBUrl, replacing only the URL.Path (or the
DSN's database field) with "/"+dbName, and serializing back to a string.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
connection/git_logging_test.go (1)
25-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkip test when
gitis unavailable instead of failing hard.At Line 26, this uses
t.Fatalf(...)whengitis not installed. For portability in minimal CI environments, this shouldt.Skipf(...)so the test degrades gracefully instead of reporting a failure.Suggested change
if _, err := exec.LookPath("git"); err != nil { - t.Fatalf("git executable not found in PATH: %v", err) + t.Skipf("git executable not found in PATH: %v", err) }🤖 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 `@connection/git_logging_test.go` around lines 25 - 27, Replace the hard failure when git isn't available with a test skip: in connection/git_logging_test.go change the exec.LookPath("git") error branch to call t.Skipf(...) instead of t.Fatalf(...), so the test function (the test using exec.LookPath("git")) will be skipped gracefully when the git executable is not found in PATH and will include the error in the skip message for debugging.
🧹 Nitpick comments (1)
PROPERTIES.md (1)
169-169: 💤 Low valueMinor grammar: compound adjectives should be hyphenated.
📝 Proposed fixes
-| `--otel-service-name` | caller supplied | OpenTelemetry service name. | +| `--otel-service-name` | caller-supplied | OpenTelemetry service name. |And at line 200:
-| `log.level.http.har` | string | unset | Enables HAR capture for HTTP at `debug`; includes full body capture at `trace`. | +| `log.level.http.har` | string | unset | Enables HAR capture for HTTP at `debug`; includes full-body capture at `trace`. |🤖 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 `@PROPERTIES.md` at line 169, The table entry for `--otel-service-name` uses the unhyphenated compound adjective "caller supplied"; update the text to use a hyphenated form ("caller-supplied") so the cell reads "`--otel-service-name` | caller-supplied | OpenTelemetry service name." and ensure any other occurrences of "caller supplied" in PROPERTIES.md are also hyphenated consistently.
🤖 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 `@Makefile`:
- Around line 101-105: The Makefile target PROPERTIES.md invokes the claude CLI
without verifying it exists; add a pre-check (like using `command -v claude` or
an equivalent guard) before the claude invocation in the PROPERTIES.md recipe so
it prints a clear, actionable error and exits if claude is missing; update the
PROPERTIES.md target (the recipe that runs claude -p ... | captain) to perform
this validation and fail fast with a helpful message when claude is not found.
In `@PROPERTIES.md`:
- Around line 241-242: The `<event>.debug` and `<event>.trace` entries are
contradictory (they claim enabling when set to `off`/`false`); first verify
whether the async event consumer code interprets these flags inverted—if the
consumer currently enables logging when the flag is false, change the consumer
logic so that true/on enables debug/trace (and update any flag parsing/negation
in the consumer where these flags are read); otherwise, if the code is correct,
update PROPERTIES.md to state that setting `<event>.debug` and `<event>.trace`
to `true`/`on` enables logging (default `false`) and remove the “off/false”
wording. Ensure both the code (if changed) and the docs consistently reflect
that true/on enables the respective logging.
---
Duplicate comments:
In `@connection/git_logging_test.go`:
- Around line 25-27: Replace the hard failure when git isn't available with a
test skip: in connection/git_logging_test.go change the exec.LookPath("git")
error branch to call t.Skipf(...) instead of t.Fatalf(...), so the test function
(the test using exec.LookPath("git")) will be skipped gracefully when the git
executable is not found in PATH and will include the error in the skip message
for debugging.
---
Nitpick comments:
In `@PROPERTIES.md`:
- Line 169: The table entry for `--otel-service-name` uses the unhyphenated
compound adjective "caller supplied"; update the text to use a hyphenated form
("caller-supplied") so the cell reads "`--otel-service-name` | caller-supplied |
OpenTelemetry service name." and ensure any other occurrences of "caller
supplied" in PROPERTIES.md are also hyphenated consistently.
🪄 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
Run ID: 874a5ce9-f7e0-41c7-b588-7f1469899e62
📒 Files selected for processing (7)
MakefilePROPERTIES.mdTaskfile.yamlconnection/git_logging_test.goquery/config_tree.goquery/resource_selector.gotests/config_access_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- Taskfile.yaml
- tests/config_access_test.go
- query/config_tree.go
Comment on lines
+101
to
+105
| .PHONY: PROPERTIES.md | ||
| PROPERTIES.md: captain | ||
| claude -p --permission-mode acceptEdits --verbose --output-format stream-json --model sonnet \ | ||
| "/properties-doc Refresh PROPERTIES.md and PROPERTIES.schema.json from the current source tree. Cross-reference ../incident-commander, ../config-db, ../canary-checker, ../flanksource-ui, and ../commons when present. Run in update mode and preserve hand-written prose sections." \ | ||
| | captain |
Contributor
There was a problem hiding this comment.
claude CLI is not validated before use.
captain gets an install guard (command -v), but the claude CLI invoked on line 103 has no equivalent check. A missing claude binary will fail with an unhelpful command not found error instead of an actionable message.
🛡️ Proposed fix
.PHONY: PROPERTIES.md
PROPERTIES.md: captain
+ `@command` -v claude >/dev/null || (echo "Claude CLI is required. Install it from https://docs.anthropic.com/en/docs/claude-code/cli-usage" && exit 1)
claude -p --permission-mode acceptEdits --verbose --output-format stream-json --model sonnet \
"/properties-doc Refresh PROPERTIES.md and PROPERTIES.schema.json from the current source tree. Cross-reference ../incident-commander, ../config-db, ../canary-checker, ../flanksource-ui, and ../commons when present. Run in update mode and preserve hand-written prose sections." \
| captain📝 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
| .PHONY: PROPERTIES.md | |
| PROPERTIES.md: captain | |
| claude -p --permission-mode acceptEdits --verbose --output-format stream-json --model sonnet \ | |
| "/properties-doc Refresh PROPERTIES.md and PROPERTIES.schema.json from the current source tree. Cross-reference ../incident-commander, ../config-db, ../canary-checker, ../flanksource-ui, and ../commons when present. Run in update mode and preserve hand-written prose sections." \ | |
| | captain | |
| .PHONY: PROPERTIES.md | |
| PROPERTIES.md: captain | |
| `@command` -v claude >/dev/null || (echo "Claude CLI is required. Install it from https://docs.anthropic.com/en/docs/claude-code/cli-usage" && exit 1) | |
| claude -p --permission-mode acceptEdits --verbose --output-format stream-json --model sonnet \ | |
| "/properties-doc Refresh PROPERTIES.md and PROPERTIES.schema.json from the current source tree. Cross-reference ../incident-commander, ../config-db, ../canary-checker, ../flanksource-ui, and ../commons when present. Run in update mode and preserve hand-written prose sections." \ | |
| | captain |
🤖 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 `@Makefile` around lines 101 - 105, The Makefile target PROPERTIES.md invokes
the claude CLI without verifying it exists; add a pre-check (like using `command
-v claude` or an equivalent guard) before the claude invocation in the
PROPERTIES.md recipe so it prints a clear, actionable error and exits if claude
is missing; update the PROPERTIES.md target (the recipe that runs claude -p ...
| captain) to perform this validation and fail fast with a helpful message when
claude is not found.
Comment on lines
+241
to
+242
| | `<event>.debug` | bool/off switch | `false` | Enables debug logging for a named async event consumer when set to `off`/`false` by current code. | | ||
| | `<event>.trace` | bool/off switch | `false` | Enables trace logging for a named async event consumer when set to `off`/`false` by current code. | |
Contributor
There was a problem hiding this comment.
Contradictory property descriptions for <event>.debug and <event>.trace.
Both rows say "Enables … logging … when set to off/false by current code" — enabling a feature when set to its off value is self-contradictory. Either the documentation should say true/on, or this is documenting an inverted-logic bug in the consumer code that should be fixed rather than just noted.
Please clarify which is the case and correct accordingly.
📝 Proposed fix (if the description is simply inverted)
-| `<event>.debug` | bool/off switch | `false` | Enables debug logging for a named async event consumer when set to `off`/`false` by current code. |
-| `<event>.trace` | bool/off switch | `false` | Enables trace logging for a named async event consumer when set to `off`/`false` by current code. |
+| `<event>.debug` | bool/off switch | `false` | Enables debug logging for a named async event consumer when set to `true`/`on`. |
+| `<event>.trace` | bool/off switch | `false` | Enables trace logging for a named async event consumer when set to `true`/`on`. |📝 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
| | `<event>.debug` | bool/off switch | `false` | Enables debug logging for a named async event consumer when set to `off`/`false` by current code. | | |
| | `<event>.trace` | bool/off switch | `false` | Enables trace logging for a named async event consumer when set to `off`/`false` by current code. | | |
| | `<event>.debug` | bool/off switch | `false` | Enables debug logging for a named async event consumer when set to `true`/`on`. | | |
| | `<event>.trace` | bool/off switch | `false` | Enables trace logging for a named async event consumer when set to `true`/`on`. | |
🤖 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 `@PROPERTIES.md` around lines 241 - 242, The `<event>.debug` and
`<event>.trace` entries are contradictory (they claim enabling when set to
`off`/`false`); first verify whether the async event consumer code interprets
these flags inverted—if the consumer currently enables logging when the flag is
false, change the consumer logic so that true/on enables debug/trace (and update
any flag parsing/negation in the consumer where these flags are read);
otherwise, if the code is correct, update PROPERTIES.md to state that setting
`<event>.debug` and `<event>.trace` to `true`/`on` enables logging (default
`false`) and remove the “off/false” wording. Ensure both the code (if changed)
and the docs consistently reflect that true/on enables the respective logging.
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
2 participants
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 by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores