Skip to content

[code-simplifier] simplify: extract traverseObjectTree and hasLabelIntentMetadata helpers #47908

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
pelikhan merged 2 commits into from
Jul 25, 2026
+52 −38
Merged

[code-simplifier] simplify: extract traverseObjectTree and hasLabelIntentMetadata helpers #47908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c1626a
simplify: extract traverseObjectTree helper and hasLabelIntentMetadat…
github-actions[bot] Jul 25, 2026
7c4aa71
fix: resolve CJS typecheck failure and simplify reviewer nits
Copilot Jul 25, 2026
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
14 changes: 11 additions & 3 deletions actions/setup/js/add_labels.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ const { withRetry, RATE_LIMIT_RETRY_CONFIG } = require("./error_recovery.cjs");
const { resolveInvocationContext } = require("./invocation_context_helpers.cjs");
const { normalizeIssueIntentLabelInputs } = require("./issue_intents.cjs");

/**
* @param {{ rationale?: string, confidence?: string, suggest?: boolean } | null | undefined} spec
* @returns {boolean}
*/
function hasLabelIntentMetadata(spec) {
return Boolean(spec && (spec.rationale || spec.confidence || spec.suggest));
}

/**
* Main handler factory for add_labels
* Uses shared count-gated scaffold for max-limit enforcement.
Expand Down Expand Up @@ -131,8 +139,8 @@ const main = createCountGatedHandler({
}
const key = label.name.toLowerCase();
const existing = requestedLabelSpecByLowerName.get(key);
const newHasMetadata = Boolean(label.rationale || label.confidence || label.suggest);
const existingHasMetadata = existing && Boolean(existing.rationale || existing.confidence || existing.suggest);
const newHasMetadata = hasLabelIntentMetadata(label);

@github-actions github-actions Bot Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/codebase-design] hasLabelIntentMetadata(existing ?? null) — the ?? null coercion is unnecessary since the function already guards with spec &&.

💡 Simplification

hasLabelIntentMetadata starts with spec && (...), so passing undefined is handled identically to null. The ?? null adds noise without safety benefit.

const existingHasMetadata = hasLabelIntentMetadata(existing);

This aligns with the third call site at line 225 which passes labelSpec directly without coercion.

@copilot please address this.

Copilot AI Jul 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Updated in 7c4aa71. I removed the ?? null call-site coercion and adjusted the helper JSDoc to accept undefined, preserving behavior while keeping the call clean.

const existingHasMetadata = hasLabelIntentMetadata(existing);
if (!existing || (!existingHasMetadata && newHasMetadata)) {
requestedLabelSpecByLowerName.set(key, label);
}
Expand Down Expand Up @@ -217,7 +225,7 @@ const main = createCountGatedHandler({

const labelsRequestPayload = uniqueLabels.map(name => {
const labelSpec = requestedLabelSpecByLowerName.get(name.toLowerCase()) ?? { name };
const hasIntentMetadata = Boolean(labelSpec.rationale || labelSpec.confidence || labelSpec.suggest);
const hasIntentMetadata = hasLabelIntentMetadata(labelSpec);
return issueIntentEnabled && hasIntentMetadata ? labelSpec : labelSpec.name;
});

Expand Down
76 changes: 41 additions & 35 deletions actions/setup/js/ai_credits_context.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,50 +94,63 @@ function resolveFirewallAuditLogPath(auditJsonlPathOverride) {
}

/**
* Depth-first traversal of a nested object, calling visitor for each [key, value] pair.
* Traversal stops early if visitor returns true.
*
* @param {unknown} entry
* @returns {string}
* @param {(key: string, value: unknown) => boolean | void} visitor - return true to stop early
* @returns {boolean} true if visitor stopped traversal early
*/
function parseMaxAICreditsFromAuditEntry(entry) {
if (!entry || typeof entry !== "object") return "";
function traverseObjectTree(entry, visitor) {
if (!entry || typeof entry !== "object") return false;
const stack = [entry];
while (stack.length > 0) {
const node = stack.pop();
if (!node || typeof node !== "object") continue;
for (const [key, value] of Object.entries(node)) {
if (MAX_AI_CREDITS_FIELDS.has(key)) {
const parsed = parsePositiveNumberString(value);
if (parsed) return parsed;
}
if (visitor(key, value) === true) return true;
if (value && typeof value === "object") stack.push(value);
}
}
return "";
return false;
}

/**
* @param {unknown} entry
* @returns {string}
*/
function parseMaxAICreditsFromAuditEntry(entry) {
let result = "";
traverseObjectTree(entry, (key, value) => {
if (MAX_AI_CREDITS_FIELDS.has(key)) {
const parsed = parsePositiveNumberString(value);
if (parsed) {
result = parsed;
return true;
}
}
return false;
});
return result;
}

/**
* @param {unknown} entry
* @returns {{ aiCredits: string, rateLimitError: boolean }}
*/
function parseAICreditsErrorInfoFromAuditEntry(entry) {
if (!entry || typeof entry !== "object") return { aiCredits: "", rateLimitError: false };
const stack = [entry];
let aiCredits = "";
let rateLimitError = false;
while (stack.length > 0) {
const node = stack.pop();
if (!node || typeof node !== "object") continue;
for (const [key, value] of Object.entries(node)) {
if (AI_CREDITS_FIELDS.has(key)) {
const parsed = parsePositiveNumberString(value);
if (parsed) aiCredits = parsed;
}
if (AI_CREDITS_RATE_LIMIT_ERROR_FIELDS.has(key) && isTrueLike(value)) rateLimitError = true;
if (AI_CREDITS_RATE_LIMIT_TEXT_FIELDS.has(key) && typeof value === "string") {
if (AI_CREDITS_RATE_LIMIT_PATTERNS.some(pattern => pattern.test(value))) rateLimitError = true;
}
if (value && typeof value === "object") stack.push(value);
traverseObjectTree(entry, (key, value) => {
if (AI_CREDITS_FIELDS.has(key)) {
const parsed = parsePositiveNumberString(value);
if (parsed) aiCredits = parsed;

@github-actions github-actions Bot Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fragile early-stop idiom: value === UNKNOWN_MODEL_AI_CREDITS_TYPE || undefined returns undefined on mismatch, only working coincidentally because traverseObjectTree checks === true.

💡 Suggested fix

Use an explicit callback instead:

function parseUnknownModelAICreditsFromAuditEntry(entry) {
  return traverseObjectTree(entry, (_key, value) => {
    if (value === UNKNOWN_MODEL_AI_CREDITS_TYPE) return true;
  });
}

When equality fails, false || undefined evaluates to undefined (not false), which happens to not trigger the early-stop only because traverseObjectTree uses strict === true. If that check is ever relaxed to truthy, this still works — but the intent is completely invisible. Making the return true explicit costs one line and removes the dependency on an internal implementation detail.

Copilot AI Jul 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Addressed in 7c4aa71 with the same explicit callback change in parseUnknownModelAICreditsFromAuditEntry (if (...) return true).

}
}
if (AI_CREDITS_RATE_LIMIT_ERROR_FIELDS.has(key) && isTrueLike(value)) rateLimitError = true;
if (AI_CREDITS_RATE_LIMIT_TEXT_FIELDS.has(key) && typeof value === "string") {
if (AI_CREDITS_RATE_LIMIT_PATTERNS.some(pattern => pattern.test(value))) rateLimitError = true;
}
});
return { aiCredits, rateLimitError };
}

Expand Down Expand Up @@ -257,17 +270,10 @@ function parseMaxAICreditsExceededFromAuditLog(auditJsonlPathOverride) {
* @returns {boolean}
*/
function parseUnknownModelAICreditsFromAuditEntry(entry) {
if (!entry || typeof entry !== "object") return false;
const stack = [entry];
while (stack.length > 0) {
const node = stack.pop();
if (!node || typeof node !== "object") continue;
for (const [, value] of Object.entries(node)) {
if (value === UNKNOWN_MODEL_AI_CREDITS_TYPE) return true;
if (value && typeof value === "object") stack.push(value);
}
}
return false;
return traverseObjectTree(entry, (_key, value) => {
if (value === UNKNOWN_MODEL_AI_CREDITS_TYPE) return true;
return false;
});
}

/**

@github-actions github-actions Bot Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/codebase-design] The || undefined idiom is correct but non-obvious — readers may not immediately see why false is avoided here.

💡 Clearer alternatives

The visitor contract requires returning true to stop early; any other value (including undefined) continues traversal. Returning false also works (since false !== true), but the intent is obscured by using || undefined to coerce a falsy result.

Consider:

return traverseObjectTree(entry, (_key, value) => {
  if (value === UNKNOWN_MODEL_AI_CREDITS_TYPE) return true;
});

or explicitly ternary:

return traverseObjectTree(entry, (_key, value) =>
  value === UNKNOWN_MODEL_AI_CREDITS_TYPE ? true : undefined
);

Either form makes the early-exit signal explicit and avoids the || undefined surprise.

@copilot please address this.

Copilot AI Jul 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Updated in 7c4aa71. I replaced the || undefined callback with an explicit early-return form to make the intent clear and avoid the fragile idiom.

Expand Down
Loading