Skip to content

Add bot auth telemetry skip contracts #1869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stranske merged 1 commit into from
Apr 26, 2026
Merged

Add bot auth telemetry skip contracts #1869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions .github/scripts/__tests__/bot-comment-auth-coverage.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,73 @@ test('warns when required organic bot auth evidence is missing', () => {
);
});

test('skips reusable organic requirement when the wrapper intentionally skipped it', () => {
const records = [
record('agents-bot-comment-handler-wrapper', 'client-id', 501, {
event_name: 'pull_request',
reusable_invocation_expected: false,
reusable_invocation_reason: 'wrapper-skipped',
}),
record('agents-bot-comment-handler-wrapper', 'client-id', 502, {
event_name: 'workflow_run',
reusable_invocation_expected: 'false',
reusable_invocation_reason: 'wrapper-skipped',
}),
];
const report = summarizeBotCommentAuthCoverage(records, {
required_organic_events: 'pull_request,workflow_run',
organic_components: 'agents-bot-comment-handler-wrapper,reusable-bot-comment-handler',
organic_expected_mode: 'client-id',
reusable_expected_mode: '',
});
const markdown = formatBotCommentAuthCoverageMarkdown(report);

assert.equal(report.organic_evidence.status, 'pass');
assert.deepEqual(report.organic_evidence.blockers, []);
assert.deepEqual(
report.organic_evidence.skipped_requirements.map((item) => `${item.component}:${item.event_name}`),
[
'reusable-bot-comment-handler:pull_request',
'reusable-bot-comment-handler:workflow_run',
]
);
assert.equal(
report.enforcement.blockers.some((blocker) =>
blocker.startsWith('missing-organic-reusable-bot-comment-handler')
),
false
);
assert.match(
markdown,
/Skipped organic requirements: reusable-bot-comment-handler\/pull_request, reusable-bot-comment-handler\/workflow_run/
);
});

test('requires reusable organic evidence when the wrapper expected to call it', () => {
const report = summarizeBotCommentAuthCoverage(
[
record('agents-bot-comment-handler-wrapper', 'client-id', 503, {
event_name: 'pull_request',
reusable_invocation_expected: true,
}),
],
{
required_organic_events: 'pull_request',
organic_components: 'agents-bot-comment-handler-wrapper,reusable-bot-comment-handler',
organic_expected_mode: 'client-id',
reusable_expected_mode: '',
}
);

assert.equal(report.organic_evidence.status, 'warning');
assert.ok(
report.enforcement.blockers.includes(
'missing-organic-reusable-bot-comment-handler-pull_request'
)
);
assert.deepEqual(report.organic_evidence.skipped_requirements, []);
});

test('keeps explicit zero parse errors and reports missing organic evidence as no-data without auth records', () => {
const organic = summarizeOrganicEvidence([], {
required_organic_events: 'pull_request',
Expand Down
29 changes: 29 additions & 0 deletions .github/scripts/__tests__/weekly-metrics-artifacts.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const {
collectRepoArtifacts,
formatArtifactTsv,
formatSelectionMarkdown,
missingPriorityFamilies,
normalizeSelectionOptions,
selectMetricsArtifacts,
} = require('../weekly_metrics_artifacts.js');
Expand Down Expand Up @@ -96,6 +97,10 @@ test('selects only recent matching artifacts with a machine-readable report', ()
'keepalive-metrics': 1,
'review-thread-terminal-disposition': 1,
});
assert.deepEqual(report.missing_priority_families, [
'verifier-terminal-disposition',
'bot-comment-auth-coverage-reusable',
]);
assert.deepEqual(report.selected_family_counts, {
'autopilot-metrics': 1,
'bot-comment-auth-coverage-wrapper': 2,
Expand All @@ -119,6 +124,12 @@ test('builds an error report when artifact selection cannot query GitHub', () =>
assert.equal(report.status, 'error');
assert.equal(report.error_message, 'API rate limit exceeded');
assert.equal(report.selected_count, 0);
assert.deepEqual(report.missing_priority_families, [
'verifier-terminal-disposition',
'review-thread-terminal-disposition',
'bot-comment-auth-coverage-wrapper',
'bot-comment-auth-coverage-reusable',
]);
assert.deepEqual(report.selected_artifacts, []);
assert.match(markdown, /Status: error/);
assert.match(markdown, /API rate limit exceeded/);
Expand Down Expand Up @@ -182,6 +193,20 @@ test('reserves priority telemetry artifacts before filling the total cap', () =>
'review-thread-terminal-disposition': 1,
'verifier-terminal-disposition': 1,
});
assert.deepEqual(report.missing_priority_families, []);
});

test('reports priority telemetry families that are absent from the scan', () => {
const counts = new Map([
['bot-comment-auth-coverage-wrapper', 2],
['keepalive-metrics', 5],
]);

assert.deepEqual(missingPriorityFamilies(counts), [
'verifier-terminal-disposition',
'review-thread-terminal-disposition',
'bot-comment-auth-coverage-reusable',
]);
});

test('formats selected artifacts for the workflow download loop', () => {
Expand All @@ -207,6 +232,10 @@ test('formats a human-visible selector summary for weekly metrics', () => {
assert.match(markdown, /Weekly Metrics Artifact Selection/);
assert.match(markdown, /Scan cap: 5 pages x 100 artifacts/);
assert.match(markdown, /Selected artifacts: 2/);
assert.match(
markdown,
/Missing priority families: verifier-terminal-disposition, review-thread-terminal-disposition, bot-comment-auth-coverage-wrapper, bot-comment-auth-coverage-reusable/
);
assert.match(markdown, /Artifact family \| Candidates \| Selected/);
assert.match(markdown, /autopilot-metrics/);
assert.match(markdown, /keepalive-metrics/);
Expand Down
37 changes: 37 additions & 0 deletions .github/scripts/bot_comment_auth_coverage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ function normalizeRecordBoolean(value) {
return Boolean(value);
}

function normalizeOptionalRecordBoolean(value) {
if (value === null || value === undefined || cleanString(value) === '') return null;
return normalizeRecordBoolean(value);
}

function normalizeMode(value) {
const text = cleanString(value).toLowerCase();
if (['hard-block', 'hard_block', 'hard', 'block', 'blocking', 'enforce'].includes(text)) {
Expand Down Expand Up @@ -134,6 +139,12 @@ function normalizeRecord(raw = {}, sourcePath = '') {
fallback_warning_active: normalizeRecordBoolean(
raw.fallback_warning_active ?? raw.fallbackWarningActive
),
reusable_invocation_expected: normalizeOptionalRecordBoolean(
raw.reusable_invocation_expected ?? raw.reusableInvocationExpected
),
reusable_invocation_reason: cleanString(
raw.reusable_invocation_reason ?? raw.reusableInvocationReason
),
source_path: sourcePath,
};
}
Expand Down Expand Up @@ -222,6 +233,7 @@ function summarizeOrganicEvidence(records = [], options = {}) {
required_components: components,
expected_mode: expectedMode === 'unknown' ? '' : expectedMode,
event_counts: eventCounts,
skipped_requirements: [],
blockers,
status: organicChecksDisabled ? 'pass' : 'no-data',
};
Expand All @@ -240,9 +252,25 @@ function summarizeOrganicEvidence(records = [], options = {}) {
}

const blockers = [];
const skippedRequirements = [];
for (const component of components) {
for (const eventName of requiredEvents) {
const latest = latestByComponentEvent[`${component}:${eventName}`];
const latestWrapper = latestByComponentEvent[
`agents-bot-comment-handler-wrapper:${eventName}`
];
const reusableWasNotExpected = component === 'reusable-bot-comment-handler' &&
latestWrapper &&
latestWrapper.reusable_invocation_expected === false;
if (!latest && reusableWasNotExpected) {
skippedRequirements.push({
component,
event_name: eventName,
reason: latestWrapper.reusable_invocation_reason || 'wrapper-did-not-call-reusable',
wrapper_run_id: latestWrapper.run_id,
});
continue;
}
if (!latest) {
blockers.push(`missing-organic-${component}-${eventName}`);
continue;
Expand All @@ -265,6 +293,7 @@ function summarizeOrganicEvidence(records = [], options = {}) {
required_components: components,
expected_mode: expectedMode === 'unknown' ? '' : expectedMode,
event_counts: eventCounts,
skipped_requirements: skippedRequirements,
blockers,
status: blockers.length > 0 ? 'warning' : 'pass',
};
Expand Down Expand Up @@ -555,6 +584,14 @@ function formatBotCommentAuthCoverageMarkdown(report) {
if (report.organic_evidence?.required_events?.length > 0) {
lines.push(`- Required organic events: ${report.organic_evidence.required_events.join(', ')}`);
lines.push(`- Organic evidence status: ${report.organic_evidence.status}`);
const skipped = report.organic_evidence.skipped_requirements || [];
if (skipped.length > 0) {
lines.push(
`- Skipped organic requirements: ${skipped
.map((item) => `${item.component}/${item.event_name}`)
.join(', ')}`
);
}
}
if (report.enforcement.blockers.length > 0) {
lines.push(`- Blockers: ${report.enforcement.blockers.join(', ')}`);
Expand Down
8 changes: 8 additions & 0 deletions .github/scripts/weekly_metrics_artifacts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,10 @@ function sortedCountObject(counts) {
);
}

function missingPriorityFamilies(candidateFamilyCounts = new Map()) {
return PRIORITY_METRICS_FAMILIES.filter((family) => !candidateFamilyCounts.has(family));
}

function selectMetricsArtifacts(artifacts = [], options = {}) {
const config = normalizeSelectionOptions(options);
const stats = {
Expand Down Expand Up @@ -223,6 +227,7 @@ function selectMetricsArtifacts(artifacts = [], options = {}) {
...stats,
candidate_family_counts: sortedCountObject(candidateFamilyCounts),
selected_family_counts: sortedCountObject(familyCounts),
missing_priority_families: missingPriorityFamilies(candidateFamilyCounts),
selected_artifacts: selected.map((artifact) => ({
id: artifact.id,
name: artifact.name,
Expand Down Expand Up @@ -257,6 +262,7 @@ function buildSelectionErrorReport(options = {}, error = {}) {
ignored_total_limit_count: 0,
candidate_family_counts: {},
selected_family_counts: {},
missing_priority_families: [...PRIORITY_METRICS_FAMILIES],
selected_artifacts: [],
};
}
Expand All @@ -283,6 +289,7 @@ function formatSelectionMarkdown(report) {
`- Scanned artifacts: ${report.scanned_count}`,
`- Candidate artifacts: ${report.candidate_count}`,
`- Selected artifacts: ${report.selected_count}`,
`- Missing priority families: ${(report.missing_priority_families || []).join(', ') || 'none'}`,
`- Ignored: ${report.ignored_old_count} old, ${report.ignored_expired_count} expired, ` +
`${report.ignored_name_count} non-metrics, ${report.ignored_family_limit_count} over family cap, ` +
`${report.ignored_total_limit_count} over total cap`,
Expand Down Expand Up @@ -428,6 +435,7 @@ module.exports = {
collectRepoArtifacts,
formatArtifactTsv,
formatSelectionMarkdown,
missingPriorityFamilies,
normalizeSelectionOptions,
selectMetricsArtifacts,
};
92 changes: 50 additions & 42 deletions .github/workflows/agents-bot-comment-handler.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,48 +99,6 @@ jobs:
echo "workflow_app_auth_mode=${workflow_app_auth_mode}"
} >> "$GITHUB_OUTPUT"

- name: Write wrapper App auth coverage
if: always()
env:
WORKFLOW_APP_AUTH_MODE: ${{ steps.workflow-app-creds.outputs.workflow_app_auth_mode }}
WORKFLOW_APP_CLIENT_ID_CONFIGURED: ${{ steps.workflow-app-creds.outputs.client_id_configured }}
WORKFLOW_APP_LEGACY_APP_ID_CONFIGURED: ${{ steps.workflow-app-creds.outputs.legacy_app_id_configured }}
WORKFLOW_APP_PRIVATE_KEY_CONFIGURED: ${{ steps.workflow-app-creds.outputs.private_key_configured }}
run: |
mkdir -p bot-comment-auth-coverage
node <<'NODE'
const fs = require('fs');
const truthy = (value) => String(value || '').toLowerCase() === 'true';
const record = {
schema: 'workflows-bot-comment-auth-coverage/v1',
component: 'agents-bot-comment-handler-wrapper',
repository: process.env.GITHUB_REPOSITORY || '',
workflow: process.env.GITHUB_WORKFLOW || '',
run_id: process.env.GITHUB_RUN_ID || '',
run_attempt: process.env.GITHUB_RUN_ATTEMPT || '',
event_name: process.env.GITHUB_EVENT_NAME || '',
auth_mode: process.env.WORKFLOW_APP_AUTH_MODE || 'none',
client_id_configured: truthy(process.env.WORKFLOW_APP_CLIENT_ID_CONFIGURED),
legacy_app_id_configured: truthy(process.env.WORKFLOW_APP_LEGACY_APP_ID_CONFIGURED),
private_key_configured: truthy(process.env.WORKFLOW_APP_PRIVATE_KEY_CONFIGURED),
fallback_warning_active: process.env.WORKFLOW_APP_AUTH_MODE === 'legacy-app-id',
direct_jobs_covered: ['resolve', 'cleanup'],
};
fs.writeFileSync(
'bot-comment-auth-coverage/wrapper.json',
`${JSON.stringify(record, null, 2)}\n`
);
NODE

- name: Upload wrapper App auth coverage
if: always()
uses: actions/upload-artifact@v7
with:
name: bot-comment-auth-coverage-wrapper-${{ github.run_id }}
path: bot-comment-auth-coverage/wrapper.json
if-no-files-found: error
retention-days: 14

- name: Mint GitHub App Token (client ID)
id: app_token_client
uses: actions/create-github-app-token@v3
Expand Down Expand Up @@ -312,6 +270,56 @@ jobs:
core.setOutput('pr_number', prNumber || '');
core.setOutput('should_run', shouldRun ? 'true' : 'false');

- name: Write wrapper App auth coverage
if: always()
env:
WORKFLOW_APP_AUTH_MODE: ${{ steps.workflow-app-creds.outputs.workflow_app_auth_mode }}
WORKFLOW_APP_CLIENT_ID_CONFIGURED: ${{ steps.workflow-app-creds.outputs.client_id_configured }}
WORKFLOW_APP_LEGACY_APP_ID_CONFIGURED: ${{ steps.workflow-app-creds.outputs.legacy_app_id_configured }}
WORKFLOW_APP_PRIVATE_KEY_CONFIGURED: ${{ steps.workflow-app-creds.outputs.private_key_configured }}
RESOLVED_PR_NUMBER: ${{ steps.resolve.outputs.pr_number }}
REUSABLE_INVOCATION_EXPECTED: ${{ steps.resolve.outputs.should_run }}
run: |
mkdir -p bot-comment-auth-coverage
node <<'NODE'
const fs = require('fs');
const truthy = (value) => String(value || '').toLowerCase() === 'true';
const reusableExpected = truthy(process.env.REUSABLE_INVOCATION_EXPECTED);

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve unknown reusable invocation state

Treating REUSABLE_INVOCATION_EXPECTED as a strict boolean here turns an unset output into false. If steps.resolve exits unexpectedly before emitting should_run (for example, an uncaught API/script failure), this record is written as reusable_invocation_expected: false, and summarizeOrganicEvidence will skip missing reusable evidence as if the wrapper intentionally skipped it. That masks real failures in the reusable path and weakens the weekly auth-coverage signal.

Useful? React with 👍 / 👎.

const record = {
schema: 'workflows-bot-comment-auth-coverage/v1',
component: 'agents-bot-comment-handler-wrapper',
repository: process.env.GITHUB_REPOSITORY || '',
workflow: process.env.GITHUB_WORKFLOW || '',
run_id: process.env.GITHUB_RUN_ID || '',
run_attempt: process.env.GITHUB_RUN_ATTEMPT || '',
event_name: process.env.GITHUB_EVENT_NAME || '',
auth_mode: process.env.WORKFLOW_APP_AUTH_MODE || 'none',
client_id_configured: truthy(process.env.WORKFLOW_APP_CLIENT_ID_CONFIGURED),
legacy_app_id_configured: truthy(process.env.WORKFLOW_APP_LEGACY_APP_ID_CONFIGURED),
private_key_configured: truthy(process.env.WORKFLOW_APP_PRIVATE_KEY_CONFIGURED),
fallback_warning_active: process.env.WORKFLOW_APP_AUTH_MODE === 'legacy-app-id',
resolved_pr_number: process.env.RESOLVED_PR_NUMBER || '',
reusable_invocation_expected: reusableExpected,
reusable_invocation_reason: reusableExpected
? 'wrapper-should-run'
: 'wrapper-skipped',
direct_jobs_covered: ['resolve', 'cleanup'],
Comment on lines +286 to +306

Copilot AI Apr 26, 2026

Copy link

Choose a reason for hiding this comment

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

reusable_invocation_expected is always emitted as a boolean computed via truthy(). If steps.resolve.outputs.should_run is unset/empty (e.g., resolve step fails before setting outputs), this will be recorded as false rather than “unknown”, which can incorrectly trigger the downstream “skip reusable organic requirements” behavior. Consider emitting null (and an explicit reason like resolve-output-missing) when REUSABLE_INVOCATION_EXPECTED is empty, and only emitting false when it is explicitly false.

Copilot uses AI. Check for mistakes.
};
fs.writeFileSync(
'bot-comment-auth-coverage/wrapper.json',
`${JSON.stringify(record, null, 2)}\n`
);
NODE

- name: Upload wrapper App auth coverage
if: always()
uses: actions/upload-artifact@v7
with:
name: bot-comment-auth-coverage-wrapper-${{ github.run_id }}
path: bot-comment-auth-coverage/wrapper.json
if-no-files-found: error
retention-days: 14

# Call the reusable workflow
handle:
name: Handle bot comments
Expand Down
Loading
Loading