Skip to content

Add ds:certify command#10

Open
mantovanoMateo wants to merge 7 commits intothanx:mainfrom
mantovanoMateo:mateo/add-certify
Open

Add ds:certify command#10
mantovanoMateo wants to merge 7 commits intothanx:mainfrom
mantovanoMateo:mateo/add-certify

Conversation

@mantovanoMateo
Copy link
Collaborator

@mantovanoMateo mantovanoMateo commented Mar 8, 2026

Summary

  • Adds /ds:certify — validates partner API integrations against official Thanx documentation using DataDog sandbox traces
  • Uses DataDog spans (merchant discovery, API version, status codes) and logs (full request params) as evidence sources
  • Headers validated via inference rules (auth from merchant.handle resolution, Content-Type from param parsing, API version from resource_name routing)
  • Generates certification report with PASS/FAIL/WARNING/PASS (inferred) per check and specific remediation steps
  • Supports --recheck mode for re-certification of previously failed items
  • Version bump 0.3.0 → 0.4.0

How It Works

Step What It Does
1 Gather partner context (name, merchant_handle, integration type, certification history)
2a Query DataDog spans for endpoint discovery (merchant.handle, resource_name, http.status_code)
2b Query DataDog logs for request payload detail (full params including nested items/modifiers)
3 Fetch official docs for each endpoint via SearchThanx (source of truth)
4 Validate headers (inferred), request body (from logs), response handling, sequencing
5 Generate certification report (PASS/FAIL/WARNING/PASS (inferred) per check)
6 Draft partner email (certified, remediation needed, or missing evidence)
7 Propose tracking updates (partner file, action items)

Key Design Decisions

  • DataDog as evidence source — uses sandbox spans + logs instead of partner-provided payloads (objective verification)
  • Two-step data collection — spans for filtering/discovery, logs for payload detail (spans lack request body, logs lack merchant filter)
  • Header inference — DataDog doesn't store raw request headers; auth/content-type/api-version inferred from observable behavior
  • merchant_handle required — DataDog resolves raw API keys server-side; only merchant.handle is searchable
  • thanx-docs MCP is mandatory — if unavailable, command stops (no guessing)
  • Integration-type-specific checks — Loyalty API, POS, Consumer UX each have different validation rules
  • Points accrual field check — catches the common amount vs subtotal mistake

DataDog Findings (from real-world testing)

  • Spans contain: merchant.handle, merchant.id, resource_name (includes API version), http.status_code, http.method, http.url
  • Logs contain: full params object with all request body fields (nested items, modifiers, etc.)
  • NOT available: raw request headers, Merchant-Key header value, Content-Type header
  • Merchant-Key is NOT searchable in either spans or logs (resolved server-side to merchant.handle)

Test plan

  • npm test — all 55 tests pass
  • Real-world DataDog testing against /api/baskets with merchant skytabpone
  • Manual test with Loyalty API integration partner
  • Manual test with POS integration evidence
  • Verify SearchThanx queries return actionable doc references

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced partner API integration validation command for testing DataDog trace compatibility against documentation standards.
  • Documentation

    • Added detailed command documentation covering certification workflows, validation rules, and reporting procedures.
  • Chores

    • Updated release version to 0.4.0/0.4.1.

Validates partner API integrations against official Thanx documentation
(thanx-docs MCP). Accepts partner-provided payloads, headers, and endpoint
references, checks each call against docs for auth, headers, request body,
field mapping, response handling, and integration-type-specific behavior.
Generates a certification report with PASS/FAIL per check and actionable
remediation steps. Supports re-certification mode for previously failed items.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Mar 8, 2026

📝 Walkthrough

Walkthrough

Version fields bumped (package.json → 0.4.1, ds plugin manifest → 0.4.1, marketplace → 0.4.0). Added plugins/ds/commands/certify.md documenting a new Certify Partner Integration command and updated plugins/ds/README.md to list the command.

Changes

Cohort / File(s) Summary
Version Bumps
package.json, plugins/ds/.claude-plugin/plugin.json, .claude-plugin/marketplace.json
Updated manifest version fields: package.json 0.3.0 → 0.4.1, plugins/ds/.claude-plugin/plugin.json 0.3.0 → 0.4.1, marketplace metadata 0.1.0 → 0.4.0. No other config edits.
New Documentation
plugins/ds/commands/certify.md, plugins/ds/README.md
Added comprehensive Certify Partner Integration command doc (end-to-end workflow, data capture rules, validation/reporting, guardrails) and README entry /ds:certify.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI as Certify CLI
    participant Datadog as DataDog
    participant Docs as thanx-docs MCP
    participant Tracker as Partner Tracker

    rect rgba(200,230,255,0.5)
    User->>CLI: run certify (partner-id, flags)
    CLI->>Tracker: fetch partner context
    end

    rect rgba(200,255,200,0.5)
    CLI->>Datadog: query API traces for partner
    Datadog-->>CLI: return traces
    end

    rect rgba(255,230,200,0.5)
    CLI->>Docs: fetch endpoint documentation
    Docs-->>CLI: return docs/schema
    end

    rect rgba(240,200,255,0.5)
    CLI->>CLI: validate traces vs docs (auth, method, body, responses, sequencing)
    CLI-->>User: generate certification report + draft communications
    CLI->>Tracker: update partner record with results
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 I hopped the versions, soft and fleet,

A certify guide for partners to meet.
Traces, docs, a careful peep—
I stitched the rules before I sleep.
Hop on—the integration leap!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title 'Add ds:certify command' directly and clearly summarizes the main change: introducing a new /ds:certify command for validating partner API integrations. It is concise, specific, and matches the primary objective.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Important

Merge conflicts detected (Beta)

  • Resolve merge conflict in branch mateo/add-certify
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…loads

Partner provides endpoints + credentials, command pulls actual API calls
from DataDog (spans + logs) and validates those against thanx-docs MCP.
This validates what they actually sent, not what they say they sent.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
plugins/ds/commands/certify.md (1)

362-364: Consider using a role-based reference instead of a person's name.

Referencing "Darren verification" ties this command to a specific individual. If that person's role changes, this becomes stale. Consider using a role-based reference like "API team lead verification" or "documentation owner verification" for better maintainability.

📝 Suggested change
-4. **Keystone answers on undocumented behavior need verification.** If a field or
-   behavior is not in the docs but found in code, flag it as needing Darren
-   verification before including in the certification result.
+4. **Keystone answers on undocumented behavior need verification.** If a field or
+   behavior is not in the docs but found in code, flag it as needing API team
+   verification before including in the certification result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/ds/commands/certify.md` around lines 362 - 364, Replace the
hard-coded person reference "Darren verification" with a role-based reference to
avoid staleness; locate the phrase "Darren verification" in the certify.md text
and change it to a neutral role such as "API team lead verification" or
"documentation owner verification" (or similar role agreed on by the team) so
the line "Keystone answers on undocumented behavior need verification" reads
that it requires "API team lead verification" instead of naming Darren.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@plugins/ds/commands/certify.md`:
- Around line 362-364: Replace the hard-coded person reference "Darren
verification" with a role-based reference to avoid staleness; locate the phrase
"Darren verification" in the certify.md text and change it to a neutral role
such as "API team lead verification" or "documentation owner verification" (or
similar role agreed on by the team) so the line "Keystone answers on
undocumented behavior need verification" reads that it requires "API team lead
verification" instead of naming Darren.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 239361a1-9349-42c6-8307-a71905e397fd

📥 Commits

Reviewing files that changed from the base of the PR and between 7f9fbac and 8ebeb83.

📒 Files selected for processing (3)
  • package.json
  • plugins/ds/.claude-plugin/plugin.json
  • plugins/ds/commands/certify.md

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@plugins/ds/commands/certify.md`:
- Around line 25-28: Update the certify command docs and implementation to make
--recheck deterministic: add an explicit step that loads prior certification
results from a canonical source (e.g., the configured or well-known prior report
file) when --recheck is provided, and if that load fails or no prior report
exists, fail fast with a clear non-zero exit and user-facing error explaining
the missing prior report; document the canonical source and the fail-fast
behavior in the certify command help alongside the --recheck and --type flags so
callers know where prior failures are read from and what to do if none are
found.
- Around line 15-23: The /ds:certify command currently asks for secrets via the
endpoints_and_credentials parameter; change the command signature and help text
to prohibit pasting secrets by renaming and rephrasing parameters (e.g., accept
endpoints_and_credential_refs or endpoints_and_lookup_ids) and require only
non-secret identifiers plus a secure lookup path or evidence token, or require a
manual evidence upload step instead of inline secrets; update any validation in
the command handler (the function that parses /ds:certify) to reject inputs that
look like API keys/OAuth secrets and add clear guidance in the command help text
and docs to instruct operators to provide stored-credential references (secret
IDs, vault paths, evidence artifacts) rather than actual credentials, and apply
the same change to the other occurrences noted (the duplicate usage around lines
51-54).
- Around line 117-123: The document currently contradicts itself between "Step
2" (do not proceed without API evidence) and "Rule 3" (allow partner-provided
samples); pick one explicit behavior and make both places consistent: either (A)
require stopping without Datadog traces (update Step 2 and Rule 3 to state the
command aborts and instruct user how to obtain traces), or (B) allow manual
request/response samples as a limited certification (update Step 2 and Rule 3 to
state manual samples are acceptable only for a limited/incomplete certification,
and add a deterministic checklist and a clear banner that the result is
partial). Also update the user-facing text at the other affected location (lines
referenced by the reviewer, e.g., 394-396) to match the chosen policy and
include the required deterministic outcome wording ("limited/incomplete
certification" or "command aborted") so the workflow is unambiguous.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 50438f34-e521-481b-a8ba-8c409840b002

📥 Commits

Reviewing files that changed from the base of the PR and between 8ebeb83 and 631b47c.

📒 Files selected for processing (1)
  • plugins/ds/commands/certify.md

- Hardcode sandbox environment (certifications always run in sandbox)
- Add --recheck flag requiring path to prior report
- Add consistent fallback policy (manual samples = PARTIAL result)
- Cap DataDog queries (2 per endpoint, 10 spans per endpoint)
- Add deduplication before validation
- Define deterministic certification criteria (FAIL→NOT CERTIFIED,
  INSUFFICIENT→INCOMPLETE, all PASS→CERTIFIED)
- Add pre-output credential verification scan
- Cap --days at 30 with default of 7
- Replace name references with role references
- Save reports for --recheck reference
- Add certify entry to README
- Bump marketplace.json to 0.4.0

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (2)
plugins/ds/commands/certify.md (2)

16-24: ⚠️ Potential issue | 🟠 Major

Accept credential references, not raw secrets.

This still asks the operator to paste API keys / OAuth identifiers into command input. That exposes secrets to chat history and tool logs before Step 5 redaction runs. Please switch the argument/help text to non-secret credential references or evidence artifact IDs, and reject inputs that look like actual keys or secrets.

Also applies to: 53-56


28-30: ⚠️ Potential issue | 🟠 Major

--recheck should fail fast when the prior report cannot be loaded.

Silently falling back to a full certification changes the requested mode and can hide a bad path or stale report reference. If --recheck is provided, stop with a clear error unless the prior report exists and parses successfully. As per coding guidelines, "Keep each command step deterministic and measurable."

Also applies to: 58-61

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/ds/commands/certify.md` around lines 28 - 30, The `--recheck <path>`
re-certification mode currently falls back silently to a full certification when
the prior report cannot be loaded; update the command handling for the
`--recheck <path>` option so that if the provided path does not exist or the
prior certification report fails to parse (use the existing
report-loading/parsing routine, e.g., the function that reads/parses the prior
report), the command exits immediately with a clear, non-zero error message
instead of continuing in full-certify mode; ensure this validation happens as
soon as `--recheck` is detected (in the code path that interprets the `--recheck
<path>` flag) and surface a descriptive error like "Failed to load prior report
at <path>: <parse error>" to make the failure deterministic and measurable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@plugins/ds/commands/certify.md`:
- Around line 102-105: The deduplication step described as "group the returned
API calls by endpoint + request body structure" is too narrow and can hide
important variants; update the deduplication key used in that step (referenced
as "Deduplication" / the grouping by endpoint + body shape) to also include HTTP
method, query parameters, authentication scheme, required headers, and response
status plus any other fields that later get validated so each representative
truly covers all dimensions you validate; change the grouping logic to compose a
composite key from endpoint + body shape + method + normalized sorted query
params + auth indicator + required headers set + status code (and any other
validated fields) and use that as the uniqueness key when collapsing calls.
- Around line 173-177: The current text lets Keystone's search_code determine
PASS/FAIL for behaviors not documented; update the certify.md wording so that
search_code may be used only to INFORM the reviewer (never to decide outcomes),
remove or rewrite the sentence that treats Keystone answers as decisive, and
explicitly require marking any behavior not defined in the official docs as
INSUFFICIENT DATA / WARNING (or pausing for doc clarification) rather than
assigning PASS/FAIL; ensure the section references "search_code", "Keystone",
and the PASS/FAIL and INSUFFICIENT DATA / WARNING outcomes so reviewers follow
the corrected policy.
- Around line 257-265: The docs define PARTIAL in Step 5 but do not provide a
corresponding communication or status flow in Step 6 or Step 7; update the
certify.md flow to make PARTIAL deterministic by either (a) adding an explicit
PARTIAL partner-facing template and a status update path in Step 7 (e.g., a
"PARTIAL - manual samples used" email/status entry) or (b) explicitly mapping
PARTIAL to an existing outcome (CERTIFIED, NOT CERTIFIED, or INCOMPLETE) with a
clear rationale; modify the content around Step 5, Step 6, and Step 7 to include
the chosen path so the command behavior is fully specified and trackable.

---

Duplicate comments:
In `@plugins/ds/commands/certify.md`:
- Around line 28-30: The `--recheck <path>` re-certification mode currently
falls back silently to a full certification when the prior report cannot be
loaded; update the command handling for the `--recheck <path>` option so that if
the provided path does not exist or the prior certification report fails to
parse (use the existing report-loading/parsing routine, e.g., the function that
reads/parses the prior report), the command exits immediately with a clear,
non-zero error message instead of continuing in full-certify mode; ensure this
validation happens as soon as `--recheck` is detected (in the code path that
interprets the `--recheck <path>` flag) and surface a descriptive error like
"Failed to load prior report at <path>: <parse error>" to make the failure
deterministic and measurable.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fe6ac789-a147-4ef7-a6e2-693491925902

📥 Commits

Reviewing files that changed from the base of the PR and between 631b47c and 7fdb57d.

📒 Files selected for processing (3)
  • .claude-plugin/marketplace.json
  • plugins/ds/README.md
  • plugins/ds/commands/certify.md
✅ Files skipped from review due to trivial changes (1)
  • plugins/ds/README.md

Rewrote the certify command based on actual DataDog sandbox testing:
- Input changed from credentials to merchant_handle (DD resolves keys server-side)
- Split trace collection into spans (2a) for discovery and logs (2b) for payload
- Headers now validated via inference (auth from merchant.handle, API version from resource_name)
- Added PASS (inferred) result type for checks deduced from indirect evidence
- Expanded rules to document DataDog limitations and merchant handle requirement

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mantovanoMateo and others added 2 commits March 18, 2026 14:40
…tify

- Step 0: Validate Intake — if called with no arguments, prints a ready-to-fill
  template. If called with partial arguments, shows a checklist of what's missing.
  Only proceeds to certification when all required fields are present.
- How This Command Works — overview section explaining the three invocation modes
  and the full certification pipeline.
- Dual DataDog strategy — Partner API/Consumer UX integrations search logs directly
  by bearer token and client ID (headers are directly observable). POS/Kiosk/Ordering
  integrations search spans by merchant handle then logs for params (headers inferred).
- Step 4a updated to distinguish direct vs inferred header validation.
- Rules 6-7 updated to reflect integration-type-aware credential requirements.
- Version bump: 0.4.0 → 0.4.1

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Addresses CodeRabbit feedback: grouping only by endpoint + body shape could
merge calls that differ in HTTP method, response status, or auth, hiding
failing variants behind a passing representative.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (4)
plugins/ds/commands/certify.md (4)

174-177: ⚠️ Potential issue | 🟠 Major

--recheck fallback to full certification breaks deterministic behavior.

Line 176 to Line 177 says to fall back to full certification if prior report load/parse fails. In recheck mode, this should fail fast with a clear error and stop, otherwise --recheck semantics are ambiguous.

As per coding guidelines, "Keep each step in commands deterministic and measurable."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/ds/commands/certify.md` around lines 174 - 177, The recheck fallback
behavior must be changed to fail-fast: when the --recheck flag is provided and
the prior certification report cannot be found or parsed, stop execution and
surface a clear error instead of falling back to a full certification; update
the re-certification mode documentation and any handler that processes --recheck
to validate existence and parseability of the report up front and return an
explicit error if validation fails so --recheck semantics remain deterministic.

360-364: ⚠️ Potential issue | 🟠 Major

Keystone is still allowed to influence PASS/FAIL outcomes.

Line 363 to Line 364 says Keystone results can inform PASS/FAIL determination. For undocumented behavior, this should be WARNING/INSUFFICIENT DATA (or stop for doc clarification), not PASS/FAIL.

As per coding guidelines, "thanx-docs MCP is the single source of truth."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/ds/commands/certify.md` around lines 360 - 364, The sentence allowing
Keystone `search_code` to "inform PASS/FAIL determination" must be changed so
results cannot directly cause PASS/FAIL; update the text around the phrase
"Optionally use Keystone `search_code` (max 5 queries) to verify undocumented
behavior" and the following sentence to state that Keystone findings may only
trigger a WARNING/INSUFFICIENT DATA outcome or require stopping for doc
clarification (never a PASS/FAIL), and add that all final determinations must
defer to the thanx-docs MCP as the single source of truth; ensure the rule
"Never quote or paraphrase Keystone code search results" remains intact and
explicitly note that Keystone is only advisory for non-passing outcomes.

472-473: ⚠️ Potential issue | 🟠 Major

PARTIAL outcome is not fully wired through communication/tracking.

Step 5 defines PARTIAL (Line 472 to Line 473), but Step 6 has no PARTIAL email path and Step 7 status options at Line 631 omit PARTIAL. Add an explicit PARTIAL communication template and tracking status (or map it deterministically to an existing status).

As per coding guidelines, "Keep each step in commands deterministic and measurable."

Also applies to: 609-621, 631-631

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/ds/commands/certify.md` around lines 472 - 473, Step 5 introduces a
PARTIAL outcome but the flow never surfaces it in communications or status
tracking; update the Step 6 communication templates and Step 7 status options to
explicitly handle PARTIAL (or add a deterministic mapping from PARTIAL to an
existing status). Specifically, add a PARTIAL email/template entry in the Step 6
communication table (the PARTIAL communication template), and add PARTIAL to the
Step 7 status enumeration/tracking list (or add a clear mapping function that
maps PARTIAL → <existing_status>), ensuring the text identifiers "PARTIAL",
"Step 6 communication template", and "Step 7 status options" are consistently
used so the outcome is measurable and traceable.

45-50: ⚠️ Potential issue | 🟠 Major

Do not collect raw credentials in intake text.

Line 49 and Line 50 (plus the repeated intake template at Line 94 and Line 95) explicitly request bearer token/client ID values, which encourages secrets in chat/session logs. Require non-secret references only (vault path, secret ID, evidence artifact), and reject inline secret-looking values.

Also applies to: 62-65, 94-95, 109-110

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/ds/commands/certify.md` around lines 45 - 50, Replace the inline
secret fields "Access Token" and "Client ID" in the intake template with
non-secret reference fields (e.g., "Access Token Reference (vault path or secret
ID)" and "Client ID Reference (vault path or secret ID)") across the certify.md
template (including the repeated template blocks) and add a short validation
note to the template that instructs reviewers to reject any submissions that
include raw bearer tokens or client IDs; ensure all occurrences of the literal
strings "Access Token" and "Client ID" are updated to the reference form and
include guidance to provide evidence artifacts or vault references instead of
paste-in secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@plugins/ds/commands/certify.md`:
- Around line 206-213: The three fenced code blocks containing the datadog_logs
examples (the block starting with "datadog_logs(" shown in the diff and the
other similar blocks at the same examples) are missing a language tag; update
each fenced block in certify.md to include a language identifier (e.g., add
```text) so the blocks at the reported locations are annotated consistently,
then run the markdown linter with npm run lint:md to validate the changes.
- Around line 200-231: Strategy A (the "logs-only" approach described under
"Strategy A: Partner API / Consumer UX (search by credentials)") contradicts
Rule 3 (the requirement that "spans+logs should always both be queried"); pick
one canonical behavior and make the docs deterministic: either update Strategy A
to mandate querying both spans and logs for Partner API integrations (editing
the "Strategy A" section and any Step 2 text to call out spans+logs) or update
Rule 3 to permit logs-only for integrations that use Partner API credentials
(and add a clear conditional statement in Rule 3 referencing Partner
API/Consumer UX); ensure the text under "Strategy A" and the Rule 3 paragraph
are consistent and reference the same canonical behavior.

---

Duplicate comments:
In `@plugins/ds/commands/certify.md`:
- Around line 174-177: The recheck fallback behavior must be changed to
fail-fast: when the --recheck flag is provided and the prior certification
report cannot be found or parsed, stop execution and surface a clear error
instead of falling back to a full certification; update the re-certification
mode documentation and any handler that processes --recheck to validate
existence and parseability of the report up front and return an explicit error
if validation fails so --recheck semantics remain deterministic.
- Around line 360-364: The sentence allowing Keystone `search_code` to "inform
PASS/FAIL determination" must be changed so results cannot directly cause
PASS/FAIL; update the text around the phrase "Optionally use Keystone
`search_code` (max 5 queries) to verify undocumented behavior" and the following
sentence to state that Keystone findings may only trigger a WARNING/INSUFFICIENT
DATA outcome or require stopping for doc clarification (never a PASS/FAIL), and
add that all final determinations must defer to the thanx-docs MCP as the single
source of truth; ensure the rule "Never quote or paraphrase Keystone code search
results" remains intact and explicitly note that Keystone is only advisory for
non-passing outcomes.
- Around line 472-473: Step 5 introduces a PARTIAL outcome but the flow never
surfaces it in communications or status tracking; update the Step 6
communication templates and Step 7 status options to explicitly handle PARTIAL
(or add a deterministic mapping from PARTIAL to an existing status).
Specifically, add a PARTIAL email/template entry in the Step 6 communication
table (the PARTIAL communication template), and add PARTIAL to the Step 7 status
enumeration/tracking list (or add a clear mapping function that maps PARTIAL →
<existing_status>), ensuring the text identifiers "PARTIAL", "Step 6
communication template", and "Step 7 status options" are consistently used so
the outcome is measurable and traceable.
- Around line 45-50: Replace the inline secret fields "Access Token" and "Client
ID" in the intake template with non-secret reference fields (e.g., "Access Token
Reference (vault path or secret ID)" and "Client ID Reference (vault path or
secret ID)") across the certify.md template (including the repeated template
blocks) and add a short validation note to the template that instructs reviewers
to reject any submissions that include raw bearer tokens or client IDs; ensure
all occurrences of the literal strings "Access Token" and "Client ID" are
updated to the reference form and include guidance to provide evidence artifacts
or vault references instead of paste-in secrets.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5879eb97-12eb-44ea-8dec-6983d3a19937

📥 Commits

Reviewing files that changed from the base of the PR and between 7fdb57d and c8e7e6c.

📒 Files selected for processing (3)
  • package.json
  • plugins/ds/.claude-plugin/plugin.json
  • plugins/ds/commands/certify.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • package.json
  • plugins/ds/.claude-plugin/plugin.json

Comment on lines +200 to +231
### Strategy A: Partner API / Consumer UX (search by credentials)

For integrations using Partner API credentials (bearer token + client ID), search
**logs directly** — they contain full request headers including `Authorization` and
`X-Clientid`, plus `params`, `status`, and `path`.

```
datadog_logs(
query: "env:sandbox service:thanx-api @data.path:*[endpoint]*",
from: "now-[days]d",
to: "now",
limit: 10
)
```

From each log, capture:
- `headers.Authorization` — bearer token (use to confirm calls belong to partner)
- `headers.X-Clientid` — client ID (second confirmation)
- `headers.Accept-Version` — API version header
- `headers.Content-Type` — **directly observable** (PASS/FAIL, not inferred)
- `headers.User-Agent` — client SDK/tool
- `method`, `path` — endpoint info
- `status` — HTTP response code
- `params` — full request body (all fields, values, nested objects)
- `duration`, `db`, `view` — performance data
- `datetime` — timestamp

**Filtering:** Match logs to the partner by checking `headers.Authorization` contains
the partner's bearer token, OR `headers.X-Clientid` matches the partner's client ID.
Exclude logs from other partners sharing the same endpoint.

**Limit: 10 logs per endpoint.** Mix of successful and error responses.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Step 2 strategy and Rule 3 contradict each other.

Strategy A (Line 200 onward) is logs-only, but Rule 3 (Line 649 to Line 652) says spans+logs should always both be queried. Pick one canonical behavior per integration type and align both sections.

As per coding guidelines, "Keep each step in commands deterministic and measurable."

Also applies to: 649-652

🧰 Tools
🪛 markdownlint-cli2 (0.21.0)

[warning] 206-206: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/ds/commands/certify.md` around lines 200 - 231, Strategy A (the
"logs-only" approach described under "Strategy A: Partner API / Consumer UX
(search by credentials)") contradicts Rule 3 (the requirement that "spans+logs
should always both be queried"); pick one canonical behavior and make the docs
deterministic: either update Strategy A to mandate querying both spans and logs
for Partner API integrations (editing the "Strategy A" section and any Step 2
text to call out spans+logs) or update Rule 3 to permit logs-only for
integrations that use Partner API credentials (and add a clear conditional
statement in Rule 3 referencing Partner API/Consumer UX); ensure the text under
"Strategy A" and the Rule 3 paragraph are consistent and reference the same
canonical behavior.

Comment on lines +206 to +213
```
datadog_logs(
query: "env:sandbox service:thanx-api @data.path:*[endpoint]*",
from: "now-[days]d",
to: "now",
limit: 10
)
```
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add language identifiers to fenced code blocks to satisfy markdown lint.

The fenced blocks at Line 206, Line 241, and Line 263 are missing a language tag (MD040). Add a language (e.g., text) consistently.

As per coding guidelines, "Run markdown lint validation before every PR with npm run lint:md."

Also applies to: 241-248, 263-270

🧰 Tools
🪛 markdownlint-cli2 (0.21.0)

[warning] 206-206: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/ds/commands/certify.md` around lines 206 - 213, The three fenced code
blocks containing the datadog_logs examples (the block starting with
"datadog_logs(" shown in the diff and the other similar blocks at the same
examples) are missing a language tag; update each fenced block in certify.md to
include a language identifier (e.g., add ```text) so the blocks at the reported
locations are annotated consistently, then run the markdown linter with npm run
lint:md to validate the changes.

Learned from Altos Agency certification — Content-Type was absent but API
accepted the request. Initially marked WARNING, but docs explicitly require
it. New rule makes this deterministic: if docs say required and header is
missing, it's always FAIL regardless of API leniency.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@mantovanoMateo mantovanoMateo requested a review from a team March 18, 2026 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant