Repository navigation
Ratify lean contract and preflight triage writes #505
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
| const fs = require("fs"); | ||
| const path = require("path"); | ||
| const { execFileSync } = require("node:child_process"); | ||
| const { GH_EXEC_DEFAULTS, readTaskAuthority } = require("../../dev-backlog/scripts/lib.js"); | ||
| const { ANCHOR_PATTERN, parseAnchor } = require("./anchor.js"); | ||
|
|
||
| const { DEFAULT_BACKLOG_DIR, defaultTriageDir } = require("../../dev-backlog/scripts/execution-root.js"); | ||
|
|
@@ -103,8 +103,18 @@ function stableSerialize(value) { | |
| return JSON.stringify(value); | ||
| } | ||
|
|
||
| // Keys treat the legacy milestone alias `name=` as `milestone=`, so an alias pair in one | ||
| // report dedupes and a log written with either spelling matches a rerun with the other. | ||
| function canonicalKeyArgs(verb, normalizedArgs) { | ||
| if (verb !== "assign-milestone" || normalizedArgs.name === undefined) return normalizedArgs; | ||
| if (normalizedArgs.milestone !== undefined && normalizedArgs.milestone !== normalizedArgs.name) return normalizedArgs; | ||
| const { name, ...rest } = normalizedArgs; | ||
| return { ...rest, milestone: name }; | ||
| } | ||
|
|
||
| function buildActionKey(action) { | ||
| const normalizedArgs = action.normalizedArgs || normalizeArgs(action.args); | ||
| return `${action.verb}|${action.issueNumber}|${stableSerialize(canonicalKeyArgs(action.verb, normalizedArgs))}`; | ||
| } | ||
|
|
||
| function parseFrontmatter(text) { | ||
|
|
@@ -175,9 +185,10 @@ function parseReport(text) { | |
| normalizedArgs, | ||
| checked, | ||
| knownVerb, | ||
| key: buildActionKey({ verb: anchor.verb, issueNumber: anchor.issueNumber, normalizedArgs }), | ||
| anchorLine: index + 1, | ||
| checkboxLine: checkboxIndex + 1, | ||
| argErrors: anchor.argErrors, | ||
| }; | ||
|
|
||
| anchors.push(entry); | ||
|
|
@@ -214,6 +225,7 @@ function dedupActions(anchors) { | |
| anchorLine: anchor.anchorLine, | ||
| checkboxLine: anchor.checkboxLine, | ||
| checked: anchor.checked, | ||
| argErrors: anchor.argErrors, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
@@ -227,12 +239,115 @@ function dedupActions(anchors) { | |
| anchorLine: anchor.anchorLine, | ||
| checkboxLine: anchor.checkboxLine, | ||
| checked: anchor.checked, | ||
| argErrors: anchor.argErrors, | ||
| }); | ||
| } | ||
|
|
||
| return [...deduped.values()]; | ||
| } | ||
|
|
||
| // Keeps every gh command line under Windows's 32,767-character limit (and so under GitHub's | ||
| // comment limit and Linux's per-argument limit), so an accepted action cannot fail at spawn | ||
| // time after earlier writes. The one joined argument, the close-duplicate comment, adds only | ||
| // a safe-integer target to its reason. | ||
| const MAX_ARG_BYTES = 16000; | ||
|
|
||
| function validateAcceptedActions(actions) { | ||
| const errors = []; | ||
| const fieldsByIssue = new Map(); | ||
| const closesByIssue = new Map(); | ||
|
|
||
| for (const action of actions) { | ||
| if (!action.checked || !action.knownVerb) continue; | ||
|
|
||
| const line = action.occurrences.find((occurrence) => occurrence.checked)?.anchorLine | ||
| || action.occurrences[0].anchorLine; | ||
| const location = `line ${line}: triage:${action.verb} #${action.issueNumber}`; | ||
| const args = action.normalizedArgs; | ||
|
There was a problem hiding this comment.
When an accepted quoted value exceeds 60,000 bytes only because of leading or trailing whitespace, AGENTS.md reference: AGENTS.md:L44-L48 Useful? React with 👍 / 👎. |
||
| for (const occurrence of action.occurrences) { | ||
| if (!occurrence.checked) continue; | ||
| for (const message of occurrence.argErrors || []) { | ||
| errors.push(`line ${occurrence.anchorLine}: triage:${action.verb} #${action.issueNumber}: ${message}.`); | ||
| } | ||
| } | ||
| if (!Number.isSafeInteger(action.issueNumber) || action.issueNumber < 1) { | ||
| errors.push(`${location}: issue number must be a positive safe integer.`); | ||
| } | ||
| for (const [key, argValue] of Object.entries(args)) { | ||
| if (String(argValue).includes("\u0000")) { | ||
| errors.push(`${location}: ${key} must not contain a NUL character.`); | ||
| } | ||
| if (Buffer.byteLength(String(argValue), "utf8") > MAX_ARG_BYTES) { | ||
| errors.push(`${location}: ${key} must be at most ${MAX_ARG_BYTES} bytes.`); | ||
|
Comment on lines
+276
to
+281
There was a problem hiding this comment.
When a checked AGENTS.md reference: AGENTS.md:L44-L48 Useful? React with 👍 / 👎. |
||
| } | ||
| } | ||
| const required = { | ||
| close: ["reason"], | ||
| revisit: ["reason"], | ||
| "close-duplicate": ["target"], | ||
| "set-priority": ["value"], | ||
| "assign-milestone": [], | ||
| }[action.verb]; | ||
|
|
||
| for (const key of required) { | ||
| if (!String(args[key] || "").trim()) { | ||
| errors.push(`${location}: ${key} is required and must not be blank.`); | ||
| } | ||
| } | ||
|
|
||
| let field; | ||
| let value; | ||
| if (action.verb === "set-priority") { | ||
| field = "priority"; | ||
| value = args.value; | ||
| if (value && !["high", "medium", "low"].includes(value)) { | ||
| errors.push(`${location}: value must be high, medium, or low.`); | ||
| } | ||
| } else if (action.verb === "assign-milestone") { | ||
| field = "milestone"; | ||
| value = args.milestone === undefined ? args.name : args.milestone; | ||
|
There was a problem hiding this comment.
When two checked proposals assign the same milestone using AGENTS.md reference: AGENTS.md:L44-L48 Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Fixed in 0f073e9: a second milestone assignment for an issue is rejected in preflight whether the value differs or repeats, and legacy |
||
| if (!String(value || "").trim()) { | ||
| errors.push(`${location}: milestone is required and must not be blank.`); | ||
| } | ||
| if (args.milestone !== undefined && args.name !== undefined && args.milestone !== args.name) { | ||
| errors.push(`${location}: milestone and legacy name have different values.`); | ||
| } | ||
| } else if (action.verb === "close-duplicate") { | ||
| field = "duplicate target"; | ||
| value = args.target; | ||
| if (value && (!/^#[1-9]\d*$/.test(value) || !Number.isSafeInteger(Number(value.slice(1))))) { | ||
| errors.push(`${location}: target must be #N with a positive issue number.`); | ||
| } else if (value && Number(value.slice(1)) === action.issueNumber) { | ||
| errors.push(`${location}: target must not be the issue itself.`); | ||
| } | ||
| } | ||
|
|
||
| if (action.verb === "close" || action.verb === "close-duplicate") { | ||
| const previousClose = closesByIssue.get(action.issueNumber); | ||
| const bothDuplicates = previousClose?.verb === "close-duplicate" && action.verb === "close-duplicate"; | ||
| if (!previousClose) { | ||
| closesByIssue.set(action.issueNumber, { verb: action.verb, target: args.target, line }); | ||
| } else if (!(bothDuplicates && previousClose.target !== args.target)) { | ||
| errors.push(`${location}: more than one accepted closing action for #${action.issueNumber}; line ${previousClose.line} already closes it.`); | ||
| } | ||
| } | ||
|
|
||
| if (!field || !value) continue; | ||
| if (!fieldsByIssue.has(action.issueNumber)) fieldsByIssue.set(action.issueNumber, new Map()); | ||
| const fields = fieldsByIssue.get(action.issueNumber); | ||
|
Comment on lines
+335
to
+337
There was a problem hiding this comment.
When the same issue has two checked AGENTS.md reference: AGENTS.md:L44-L48 Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Fixed in 43b2902: preflight now rejects a second accepted closing action ( |
||
| const previous = fields.get(field); | ||
| if (previous && previous.value !== value) { | ||
| errors.push(`${location}: conflicting ${field} ${JSON.stringify(value)}; line ${previous.line} assigns ${JSON.stringify(previous.value)}.`); | ||
| } else if (previous && field !== "duplicate target") { | ||
| errors.push(`${location}: ${field} ${JSON.stringify(value)} is already assigned on line ${previous.line}; keep one anchor.`); | ||
| } else if (!previous) { | ||
| fields.set(field, { value, line }); | ||
|
Comment on lines
+338
to
+344
There was a problem hiding this comment.
When two checked AGENTS.md reference: AGENTS.md:L44-L48 Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Fixed in 0f073e9: any second priority or milestone assignment for the same issue fails preflight ("already assigned on line N; keep one anchor"), regardless of non-semantic args such as |
||
| } | ||
| } | ||
|
|
||
| return errors; | ||
| } | ||
|
|
||
| function formatRevisitComment(reason) { | ||
| return `triage: revisit — ${String(reason || "").trim()}`; | ||
| } | ||
|
|
@@ -293,7 +408,7 @@ function toGhCommands(action, context = {}) { | |
| ]; | ||
| case "assign-milestone": | ||
| return [ | ||
| ["issue", "edit", issue, "--milestone", String(args.milestone ?? args.name ?? "").trim()], | ||
|
There was a problem hiding this comment.
When an action was first applied with legacy AGENTS.md reference: AGENTS.md:L44-L48 Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Fixed in 96eb862: action keys canonicalize legacy |
||
| ]; | ||
| default: | ||
| return []; | ||
|
|
@@ -721,6 +836,26 @@ function execute(argv = process.argv.slice(2), deps = {}) { | |
| }; | ||
| } | ||
|
|
||
| const deduped = dedupActions(report.anchors); | ||
| const validationErrors = validateAcceptedActions(deduped); | ||
| if (validationErrors.length > 0) { | ||
| return { | ||
| exitCode: 1, | ||
| error: `Invalid accepted triage actions:\n${validationErrors.map((error) => `- ${error}`).join("\n")}`, | ||
| }; | ||
| } | ||
|
|
||
| if (options.apply) { | ||
| try { | ||
| const authority = readTaskAuthority(path.resolve(cwd, DEFAULT_BACKLOG_DIR)); | ||
| if (authority !== "github") { | ||
| throw new Error(`task authority is ${JSON.stringify(authority)}; triage apply requires github.`); | ||
| } | ||
| } catch (error) { | ||
| return { exitCode: 1, error: `Cannot apply triage actions: ${error.message}` }; | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| ensureApplyAllowed(options, { | ||
| confirmApply: deps.confirmApply, | ||
|
|
@@ -736,7 +871,6 @@ function execute(argv = process.argv.slice(2), deps = {}) { | |
| }; | ||
| } | ||
|
|
||
| const applyMode = options.apply ? "apply" : "dry-run"; | ||
| const result = { | ||
| report: reportPath, | ||
|
|
@@ -888,6 +1022,7 @@ module.exports = { | |
| parseFrontmatter, | ||
| parseReport, | ||
| dedupActions, | ||
| validateAcceptedActions, | ||
| formatRevisitComment, | ||
| formatDuplicateComment, | ||
| buildPriorityEditCommand, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a checked known anchor contains malformed or duplicate argument text, validation sees only the object already produced by
parseAnchorArgs; for example,reason="first" reason="second"silently keeps the latter value, whilereason="stale" garbageignoresgarbage. Both therefore pass preflight and can trigger an irreversible GitHub write despite violating the documented key-value grammar. Validate that the entire argument payload is consumed exactly once per key before accepting the action.AGENTS.md reference: AGENTS.md:L44-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0f073e9:
argSyntaxErrorsinanchor.jsrequires the whole args payload to be whitespace-separatedkey=value/key="value"pairs with each key once; a checked occurrence with a repeated key or leftover text fails preflight with its line. Covered by tests for both examples.