feat(sdk): expose generic discovery evaluation evidence - #164
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
roborev: Combined Review (
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55a26aa01f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab00ae3bdf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42178b4f71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const definition = awsDiscoveryDatasetRegistry[datasetKey]; | ||
| const resourceType = definition.resourceTypes[0] ?? definition.service; |
There was a problem hiding this comment.
Preserve each untagged resource's concrete type
When discover() enables CLDBRN-AWS-TAGGING-1 with includeEvaluationResources: true, this fallback assigns every evidence item the literal type tagging: the dataset declares no catalog resourceTypes, while its projection discards the concrete AwsUntaggedResource.resourceType retained by the hydrator. Mixed EC2, S3, and other resources therefore receive incorrect identities; carry each resource's own type into the projection.
AGENTS.md reference: packages/sdk/AGENTS.md:L24-L25
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac07dee. Account-wide tagging evidence now carries each resource's concrete Resource Explorer type, with regression coverage for mixed EC2 and S3 resources.
Acknowledged. The reported commit had no review findings. |
Acknowledged. The latest review run is in progress against the generic discovery contract. |
There was a problem hiding this comment.
💡 Codex Review
When CLDBRN-AWS-ECR-1 is requested with evaluation resources, this projection emits only the repository name even though AwsEcrRepository.arn was already retained by the hydrator. A generic consumer therefore cannot create an unambiguous AWS resource link without reconstructing an ECR-specific ARN; include the available arn and name fields in this projection.
AGENTS.md reference: packages/sdk/AGENTS.md:L21-L22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| >; | ||
| 'id' | 'name' | 'description' | 'message' | 'provider' | 'service' | 'severity' | 'supports' | ||
| > & { | ||
| serviceName: string; |
There was a problem hiding this comment.
Keep presentation labels out of the SDK contract
serviceName is a required English display label on the already-exported BuiltInRuleMetadata, so an SDK upgrade makes previously valid consumer-constructed metadata objects fail type checking and couples every metadata and evaluation consumer to SDK presentation policy. Keep the stable service key in this generic contract and let consuming applications add display labels, or at minimum make this field additive.
AGENTS.md reference: packages/sdk/AGENTS.md:L18-L20
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 982897e. Removed the English serviceName presentation label from the generic SDK metadata and evaluation contract; consumers retain the stable service key.
| tagging: 'Tagging', | ||
| }; | ||
|
|
||
| export const toBuiltInRuleMetadata = ({ |
There was a problem hiding this comment.
Document the newly exported metadata projector
toBuiltInRuleMetadata is newly exported so run-live.ts can import it, but it has no TSDoc describing its purpose, parameter, or return value. Add the required documentation or avoid exporting the helper so this exported code follows the repository contract.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 982897e. Added TSDoc covering the metadata projector's purpose, parameter, and return value.
| ); | ||
| }) | ||
| .map((logGroup) => createFindingMatch(logGroup.logGroupArn, logGroup.region, logGroup.accountId)); | ||
| .map((logGroup) => createFindingMatch(logGroup.logGroupName, logGroup.region, logGroup.accountId)); |
There was a problem hiding this comment.
Preserve the published CloudWatch ARN identity
When clients persist or map CLDBRN-AWS-CLOUDWATCH-2 findings by resourceId, this changes the published value from the log-group ARN to a bare name, so existing keys and ARN-based links stop matching after an upgrade. The activity projection now carries logGroupArn, so preserve that ARN as the finding and evaluated-resource identity instead of changing the existing finding contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 982897e. Restored the CloudWatch log-group ARN for both finding and evaluated-resource identity while retaining the log-group name as normalized display evidence.
| accountId: (latestStream?.arn ? extractAccountIdFromArn(latestStream.arn) : null) ?? discoveredAccountId, | ||
| accountId: | ||
| (latestStream?.arn ? extractAccountIdFromArn(latestStream.arn) : null) ?? discoveredResource.accountId, | ||
| ...(latestActivityTimestamp ? { lastActivityAt: new Date(latestActivityTimestamp).toISOString() } : {}), |
There was a problem hiding this comment.
Avoid reporting one stream's ingestion as group activity
When a log group receives a late-arriving event in a stream whose event timestamp is older than another stream's, the LastEventTime ordering with limit: 1 selects the other stream, so this newly exposed lastActivityAt can be older than the group's actual latest ingestion. Do not publish this value as group-wide activity unless ingestion recency is collected across the streams, or omit it from the normalized evidence.
AGENTS.md reference: packages/sdk/AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 982897e. lastActivityAt now represents only the latest stream event timestamp selected by LastEventTime; ingestion time is retained as raw dataset evidence but no longer presented as group-wide activity.
| '@cloudburn/rules': minor | ||
| '@cloudburn/sdk': minor |
There was a problem hiding this comment.
The cloudburn rules list --format json path serializes builtInRuleMetadata verbatim, so adding fields such as message changes the published CLI output even though this changeset lists only the rules and SDK packages. Include cloudburn directly so its version and release notes cover the affected user-facing package rather than relying only on a transitive dependency bump.
AGENTS.md reference: AGENTS.md:L55-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 982897e. Added the CLI package to the changeset because its JSON rule metadata output changes.
- keep presentation labels outside the generic SDK contract - preserve CloudWatch ARN identity and latest-event age semantics - make evaluation dataset selection explicit and release the CLI change
Fixed in 982897e. Added an explicit CLDBRN-AWS-CLOUDWATCH-2 evaluation dataset override and restored dependency ordering so projection selection no longer depends on array position. |
Review completed and all six actionable findings were addressed in 982897e; full verification is green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 982897eb5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lastIngestionTime: latestStream?.lastIngestionTime, | ||
| latestStreamArn: latestStream?.arn, | ||
| latestStreamName: latestStream?.logStreamName, | ||
| logGroupArn: discoveredResource.arn, |
There was a problem hiding this comment.
Keep CloudWatch evaluation IDs aligned with findings
When Resource Explorer returns the canonical log-group ARN without a trailing :*, this stores that ARN in the evaluation set, while hydrateAwsCloudWatchLogGroups() still reads DescribeLogGroups.arn—which the AWS SDK contract documents as including :*—and the rule uses that value for its finding ID. A triggered CLDBRN-AWS-CLOUDWATCH-2 result therefore cannot be joined to its evaluated resource by resourceId; project the activity through the loaded log-group inventory or otherwise normalize both paths to the same ARN. Fresh evidence in the current tree is that the attempted fix now sources these two identities from different AWS responses.
AGENTS.md reference: packages/sdk/AGENTS.md:L21-L22
Useful? React with 👍 / 👎.
Summary
CloudBurnClient.discover()Validation
pnpm verifypnpm docs:check