Skip to content

Ratify lean contract and preflight triage writes - #505

Merged
sungjunlee merged 13 commits into
mainfrom
codex/lightening-s1
Sep 29, 2026
Merged

sungjunlee merged 13 commits into
mainfrom
codex/lightening-s1

Conversation

@sungjunlee

@sungjunlee sungjunlee commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner

Accepted triage actions could reach GitHub with missing arguments or an empty milestone, and a later invalid action could fail after earlier actions had already mutated issues. The writer now validates the entire accepted set before any GitHub call or apply-log write, uses milestone= with the legacy name= fallback, and rejects non-GitHub or unreadable task authorities.

This Batch 1 change also ratifies the explicitly approved lean spec amendments from #494 in a separate spec commit. The approval record identifies patch SHA-256 c200c64147bf3dba71805dba7b9618094d0a5bebcad07be281ace0c8b59f14ff; application substitutes only the four gate placeholders with that record link and date. The next sprint-reader and outcome-baseline work remains in #496 and #497.

Validation (head 3d5d24c):

  • Full Node suite: 196 passed, 1 optional live integration skipped, 0 failures.
  • Smoke suite: 167 passed, 0 failures.
  • Every preflight regression asserts zero fake GitHub invocations and an unchanged or absent apply log, including a bad action placed after valid ones; a report with every source anchor and its Apply Checklist copy checked applies each action exactly once.

Review: GPT-6 Sol implemented #495; Cursor Grok 4.7 reviewed it (LGTM). Claude Opus 5.5 drafted #494; GPT-6 Sol reviewed revision 2 (no substantive blocker).

PR review cycle (Claude Opus 5.5 applied the fixes):

Commits: eeb0e7e is the triage correction; 47e8604 contains only the three approved spec files; the (#505 review) commits are the review fixes above. Merge with rebase so the spec commit stays separate. The three approved #503 clarifications were applied and read back exactly. The spec states the target behavior for later issues; it does not claim the runtime-retirement work is already implemented.

Closes #495
Refs #494, #504

Summary by CodeRabbit

  • 버그 수정

    • 적용 및 미리보기 전에 선택된 작업의 번호, 인자, 우선순위, 중복 지정과 종료 작업을 검증합니다. 오류나 충돌이 있으면 전체 작업을 중단하고 줄 번호와 함께 보고합니다.
    • 적용은 권한 정보가 없거나 첫 줄이 github인 경우에만 진행합니다. 그 외의 경우나 권한 정보를 읽을 수 없는 경우에는 GitHub 호출과 적용 기록을 남기기 전에 중단합니다.
    • 마일스톤 지정에 표준 milestone= 인자를 지원하며, 기존 name= 인자도 호환됩니다. 둘 다 사용했을 때 값이 다르면 거부합니다.
  • 문서

    • 지원되는 트래커의 검증 상태와 스프린트 완료 절차를 명확히 했습니다.

🤖 Generated with Claude Code

https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ

@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 0408f039-9801-45e0-98b9-5015969ba702

📥 Commits

Reviewing files that changed from the base of the PR and between 8fb2016 and 0f073e9.

📒 Files selected for processing (10)
  • .dev-backlog/sprints/_context.md
  • CHANGELOG.md
  • skills/backlog-triage/SKILL.md
  • skills/backlog-triage/references/apply.md
  • skills/backlog-triage/scripts/anchor.js
  • skills/backlog-triage/scripts/triage-apply.js
  • spec/capabilities.md
  • spec/charter.md
  • spec/system-map.md
  • tests/backlog-triage/triage-apply.test.js

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

triage-apply가 체크된 작업의 인자와 충돌을 외부 호출 및 로그 기록 전에 검증합니다. 적용 모드에서는 tracker 권한도 확인합니다. 관련 문서와 테스트를 갱신하고, Sprint 범위·배치·종료 절차에 관한 계약을 구체화합니다.

Changes

triage-apply 사전 검증

Layer / File(s) Summary
Apply 계약과 권한 규칙
skills/backlog-triage/SKILL.md, skills/backlog-triage/references/apply.md, spec/capabilities.md, spec/charter.md, spec/system-map.md, CHANGELOG.md, .dev-backlog/sprints/_context.md
milestone=을 표준 인자로 정하고, name= 호환 및 Apply 전 검증·권한 규칙을 문서화합니다. 트리아지 보고서의 파싱 범위와 authority 계약도 갱신합니다.
앵커 파싱과 작업 검증
skills/backlog-triage/scripts/anchor.js, skills/backlog-triage/scripts/triage-apply.js, tests/backlog-triage/triage-apply.test.js
앵커의 파싱되지 않은 내용과 중복 키를 오류로 보존합니다. 체크된 알려진 작업의 필수값, 형식, 길이 및 충돌을 검사하고 관련 거부 동작을 테스트합니다.
적용 실행과 milestone 호환
skills/backlog-triage/scripts/triage-apply.js, tests/backlog-triage/triage-apply.test.js
검증 뒤 적용 모드의 tracker 권한이 github인지 확인합니다. milestone=을 사용하고, 해당 키가 없을 때는 name=으로 대체합니다. 권한 및 재실행 동작을 테스트합니다.

Sprint 운영 계약

Layer / File(s) Summary
Sprint 범위와 종료 절차
spec/capabilities.md, spec/system-map.md
scope:의 디렉터리 접두사 규칙과 배치 대기 조건을 명시합니다. 종료 전 남은 작업과 처리 방침을 보고하고, 세션이 상태를 수정한 뒤 활성 트랙에서 제거됐는지 확인하는 절차를 문서화합니다.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 0f073

This change adds pre-write validation and a GitHub-only authority check to triage apply, and it clarifies the spec. No blocking issues remain, so it is ready to merge.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 0f073

The new validation and tracker-authority checks reduce unintended writes, but the promised checklist-only approval boundary is not enforced. A checked action elsewhere in a report can still reach GitHub when apply is run.

Retained concerns

  • Medium · security · observed: The newly ratified single approval surface is not enforced: a checked, valid anchor outside Apply Checklist is parsed and can be executed. This matters when a reviewer treats prose sections as non-actionable, although a checked anchor and explicit apply invocation are still required.
Security review details

Security Blast Radius

  • inferred — A qualifying off-checklist anchor can affect the issue number it names in the repository selected by the invoking GitHub CLI context. The examined call supplies no explicit repository binding, so the actual destination and actor are not established by the report or authority gate alone.

Security Findings and Attack Paths

  • inferred — If report content contains a checked, syntactically valid action outside Apply Checklist, whole-report parsing makes it eligible for a GitHub write despite the new contract. Whether untrusted issue content is copied into such an action was not established; this is not an automatic write without explicit apply consent.

Trust Boundaries and Controls

  • observed — The authority check controls whether the declared tracker permits this writer to proceed, not which GitHub repository or actor receives the mutation. The report checkbox and apply flag remain separate consent controls.

Resilience and Maintainability Implications

  • inferred — Logged partial progress supports retries, but interruption after a successful remote call and before its log append, or concurrent reads of the same log, can defeat local repeat-apply protection. This mechanism was not changed by the PR and is not retained as a newly worsened concern.

Hardening Proposals

  • proposed — Enforce the documented section boundary before accepting actions, align report-production instructions with that boundary, and cover rejection of checked off-checklist anchors before GitHub or log writes.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 3 files. (7 skipped: 7… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed 직접 연결된 #495의 코딩 요구사항을 충족합니다. triage-apply.js는 milestone=을 우선 사용하고, milestone=이 없을 때만 name=을 사용합니다. 충돌하는 별칭, 필수 인자 누락, 우선순위 오류, 잘못된 중복 대상, 중복·충돌 할당, 여러 종료 작업, 잘못된 이슈 번호와 인자 구문을 사전 검증합니다. 검증은 Gi…
Out of Scope Changes check ✅ Passed 변경 범위는 #495의 triage apply 검증, 권한 확인, 인자 문서화와 테스트에 연결됩니다. anchor.js 변경은 인자 구문 오류와 중복 키를 검증하기 위한 지원 변경입니다. SKILL.md, apply.md, CHANGELOG.md, _context.md, spec/* 변경은 canonical key, authority,…
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 lean contract 승인과 triage 쓰기 전 검증이라는 변경의 주요 내용을 정확하고 간결하게 요약합니다.
Full details: Docstring Coverage

Explanation

Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 3 files. (7 skipped: 7 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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

나는 토끼, 검증의 문을 살펴봐
체크된 앵커의 잘못된 값을 찾아
GitHub 앞에서 권한도 확인해
새 milestone 이름과 옛 이름을 함께 기억해
Sprint 길에도 새 규칙을 적어
깡충, 안전한 기록을 축하해!

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

@sungjunlee
sungjunlee marked this pull request as ready for review September 29, 2026 11:56
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-29T14:28:16.837669Z 3d5d24c New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 47e8604bd7

ℹ️ 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".

Comment thread spec/capabilities.md
### Expected Behaviors
- Default `backlog-triage` invocation is **advisory** — it produces a markdown report and never mutates GitHub state. Mutation requires `--apply`.
- Alignment Check maps every open Issue to ≥1 Objective OR surfaces it as an orphan in the report — no silent drops.
- `triage-apply` reads anchors only from `## Apply Checklist`; an anchor outside it is rejected before any write with a message naming the line.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Restrict mutation anchors to the apply checklist

When a report contains a checked anchor in a source section such as ## Obsolete Candidates, parseReport() still scans and accepts it, and --apply --yes executes it even if ## Apply Checklist is empty or unchecked. This defeats the newly declared single approval surface and is especially hazardous because the current SKILL.md still instructs agents to emit anchors throughout the report; reject anchors outside the checklist and name their lines before any write.

AGENTS.md reference: AGENTS.md:L7-L9

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Deferred by design: this PR ratifies the approved spec text as the target behavior; the checklist-only anchor rule is implemented by #502 (one approval surface for the triage report and apply), as the PR body states. Until #502 lands, the shipped triage-apply.js keeps its current any-location acceptance.

Comment on lines +288 to +290
if (!field || !value) continue;
if (!fieldsByIssue.has(action.issueNumber)) fieldsByIssue.set(action.issueNumber, new Map());
const fields = fieldsByIssue.get(action.issueNumber);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Reject duplicate closes before any write

When the same issue has two checked close anchors with different reason wording—the case explicitly declared invalid in references/apply.md—deduplication treats them as distinct, while this branch skips both because close never assigns field. Apply consequently emits two comments and two close mutations instead of failing preflight, so a harmless wording variation can cause duplicated GitHub writes; detect multiple accepted closes for an issue during validation.

AGENTS.md reference: AGENTS.md:L44-L48

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 43b2902: preflight now rejects a second accepted closing action (close or close-duplicate) for the same issue with both report line numbers, before any GitHub call or apply-log write, in dry-run and apply. Different duplicate targets keep the existing conflict message. Covered by a new test (three pairings × dry-run/apply, zero gh calls, no log).

@sungjunlee

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…review)

Two accepted `close` anchors for one issue that differ only in reason
wording, a `close` plus a `close-duplicate`, or two same-target
`close-duplicate` anchors survived dedup and produced duplicate comment and
close writes. Preflight now fails them with both report line numbers before
any GitHub call or apply-log write; different duplicate targets keep their
existing conflict message. CHANGELOG Unreleased gains the #494 and #495 lines.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 43b29025a8

ℹ️ 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".

}
} else if (action.verb === "assign-milestone") {
field = "milestone";
value = args.milestone === undefined ? args.name : args.milestone;

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 Canonicalize the legacy milestone alias before deduplication

When two checked proposals assign the same milestone using milestone="A" and legacy name="A"—for example, when a legacy source proposal is repeated canonically in the checklist—this fallback recognizes equivalent values, but the earlier raw-argument deduplication gives them different action keys. Validation therefore succeeds and apply executes the identical gh issue edit ... --milestone A mutation twice; normalize name into milestone before building action keys so aliases share one deterministic action.

AGENTS.md reference: AGENTS.md:L44-L48

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 0f073e9: a second milestone assignment for an issue is rejected in preflight whether the value differs or repeats, and legacy name= counts as milestone=, so the alias pair fails with both line numbers before any write.

sungjunlee and others added 2 commits September 29, 2026 21:47
…505 review)

The ratified spec states the epic #504 target (checklist-only triage
anchors, conservative cross-axis scope checks, a waiting next batch behind
an earlier `[~]`, session-edited sprint close). The approved spec text stays
verbatim; `_context.md` says to execute from SKILL.md and the scripts, grade
future work against spec, and retire the line when the last issue merges.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
…eger (#505 review)

`#0` (or a number beyond the safe-integer range) passed preflight, so an
earlier valid action could reach GitHub and the apply log before the bad
one failed. Preflight now names the line and stops before any write; the
aggregate test places the bad anchor after a valid revisit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 32e0dc252b

ℹ️ 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".

Comment on lines +305 to +309
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) {
fields.set(field, { value, line });

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 Reject duplicate same-value field assignments

When two checked assign-milestone anchors target the same issue and milestone but differ only in a non-semantic argument such as reason, their action keys remain distinct and this equality branch accepts both; apply then executes the identical gh issue edit --milestone mutation twice. This contradicts the duplicate rule in skills/backlog-triage/references/apply.md:106, so reject or semantically deduplicate repeated same-value assignments during preflight.

AGENTS.md reference: AGENTS.md:L44-L48

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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 reason. Covered by a new test.

@sungjunlee

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Pull request base or head changed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…ite (#505 review)

A NUL in a reason or milestone value passed preflight and then failed
inside execFileSync after earlier actions had been applied. Preflight now
names the line and key; tests cover a single action and a NUL value placed
after valid actions (zero gh calls, log unchanged).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bdf945dbd1

ℹ️ 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 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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Reject unparsed anchor argument text

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, while reason="stale" garbage ignores garbage. 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 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 0f073e9: argSyntaxErrors in anchor.js requires the whole args payload to be whitespace-separated key=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.

…ny write (#505 review)

A 1 MiB reason or milestone passed preflight and failed with E2BIG at spawn
time after earlier actions had been applied. The cap sits below GitHub's
65,536-character comment limit and Linux's 128 KiB per-argument limit
(a close-duplicate comment joins two capped values). This closes the
"passes preflight, fails deterministically at spawn" class; network,
missing-target, and GitHub-side validation failures stay with the apply
log's resume.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 177a551db1

ℹ️ 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".

Comment on lines +255 to +260
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.`);

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 Cap the rendered duplicate comment

When a checked close-duplicate supplies a target and reason that are each below 60,000 bytes, this per-value check accepts them even though formatDuplicateComment() concatenates both into one argument. For example, a 59,999-digit target plus a 60,000-byte reason produces a 120,015-byte comment body, exceeding the 65,536-character ceiling this block is explicitly intended to enforce; if this action follows valid actions, gh issue comment can fail only after those earlier mutations have occurred. Validate the rendered comment size, or impose a realistic bound on target, during whole-set preflight.

AGENTS.md reference: AGENTS.md:L44-L48

Useful? React with 👍 / 👎.

…review)

Codex connector review: `reason="a" reason="b"` kept the last value and
trailing text was ignored, so a malformed accepted anchor could still write;
and two same-value milestone or priority anchors (legacy `name=` alias, or
a different reason) produced the same edit twice. Preflight now rejects a
repeated key, unparsed argument text, and any second priority or milestone
assignment for an issue. apply.md lists the preflight rules; the CHANGELOG
line points there instead of enumerating them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
@sungjunlee

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…er the cap (#505 review)

A 59,999-digit target passed the per-value cap, and joined with a
60,000-byte reason the duplicate comment exceeded GitHub's comment limit.
The target must now be #N with N a positive safe integer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0f073e96f9

ℹ️ 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 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;

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 Measure argument limits before trimming

When an accepted quoted value exceeds 60,000 bytes only because of leading or trailing whitespace, normalizeArgs() strips that whitespace before this assignment, so the byte-limit check sees the shortened value and accepts it. The original unbounded value remains in action.args and is persisted by buildLogEntry() after the GitHub mutation; if that oversized append fails, the mutation is left unrecorded and a retry can repeat it. Validate the raw parsed values before trimming, or consistently store the validated normalized values.

AGENTS.md reference: AGENTS.md:L44-L48

Useful? React with 👍 / 👎.

…action (#505 review)

Grok round 6 P3s: nothing asserted that a report with every source anchor
and its Apply Checklist copy checked still dedupes and applies each action
once; and the issue-number message now says "positive safe integer",
matching the duplicate-target rule.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8941ad9f78

ℹ️ 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".

case "assign-milestone":
return [
["issue", "edit", issue, "--milestone", String(args.name || "").trim()],
["issue", "edit", issue, "--milestone", String(args.milestone ?? args.name ?? "").trim()],

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 Canonicalize milestone aliases in persisted action keys

When an action was first applied with legacy name="A" and the report is later rewritten using the newly canonical milestone="A", readApplyLog() reconstructs the old key from {name: "A"} while the current action uses {milestone: "A"}. Fresh evidence after the duplicate-assignment fix is that these two sequential runs still execute the identical gh issue edit ... --milestone A mutation twice instead of returning already-applied; rejecting aliases that coexist in one report does not protect reruns after report migration. Canonicalize the alias before building and persisting action keys.

AGENTS.md reference: AGENTS.md:L44-L48

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 96eb862: action keys canonicalize legacy name= to milestone= (when they do not disagree), so an alias pair in one report is one action and a rerun after the spelling change is already-applied. Test covers both.

…iew)

Codex connector: a log written with `name="A"` did not match a rerun of the
same assignment rewritten as `milestone="A"`, so the edit ran again. Action
keys now treat `name=` as `milestone=` (when they do not disagree), so an
alias pair in one report is one action and reruns across the spelling
change are `already-applied`. Other verbs' keys are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 96eb862314

ℹ️ 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".

// Below GitHub's 65,536-character comment limit and Linux's 128 KiB 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 = 60000;

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 Cap arguments for the Windows command-line limit

On native Windows, execFileSync ultimately uses a command line limited to roughly 32,767 UTF-16 characters, so an accepted action with a 40,000-character ASCII reason passes this 60,000-byte check but fails before gh starts. If that action follows another accepted action, the earlier GitHub mutation has already occurred, defeating the preflight guarantee this limit is intended to provide; use a conservative cross-platform limit or execute large comment bodies through a non-command-line input path.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 05af28c: the per-value cap is now 16,000 UTF-8 bytes, which keeps the longest gh command line (the close-duplicate comment: safe-integer target plus reason) under the Windows 32,767-character limit.

sungjunlee and others added 2 commits September 29, 2026 23:13
…line (#505 review)

Codex connector: native Windows limits a command line to about 32,767
UTF-16 characters, so a 40,000-character reason passed the 60,000-byte
cap and failed at spawn after earlier writes. 16,000 UTF-8 bytes keeps
the longest gh command (close-duplicate comment) under that limit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
Grok round 7 P3: apply.md and backlog-triage SKILL.md described the key as
(verb, issueNumber, normalizedArgs) only; they now say a non-conflicting
legacy `name=` is keyed as `milestone=`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
@sungjunlee
sungjunlee merged commit 33a6005 into main Sep 29, 2026
3 checks passed
sungjunlee added a commit that referenced this pull request Sep 29, 2026
…review)

Two accepted `close` anchors for one issue that differ only in reason
wording, a `close` plus a `close-duplicate`, or two same-target
`close-duplicate` anchors survived dedup and produced duplicate comment and
close writes. Preflight now fails them with both report line numbers before
any GitHub call or apply-log write; different duplicate targets keep their
existing conflict message. CHANGELOG Unreleased gains the #494 and #495 lines.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
sungjunlee added a commit that referenced this pull request Sep 29, 2026
…505 review)

The ratified spec states the epic #504 target (checklist-only triage
anchors, conservative cross-axis scope checks, a waiting next batch behind
an earlier `[~]`, session-edited sprint close). The approved spec text stays
verbatim; `_context.md` says to execute from SKILL.md and the scripts, grade
future work against spec, and retire the line when the last issue merges.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
sungjunlee added a commit that referenced this pull request Sep 29, 2026
…eger (#505 review)

`#0` (or a number beyond the safe-integer range) passed preflight, so an
earlier valid action could reach GitHub and the apply log before the bad
one failed. Preflight now names the line and stops before any write; the
aggregate test places the bad anchor after a valid revisit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
sungjunlee added a commit that referenced this pull request Sep 29, 2026
…ite (#505 review)

A NUL in a reason or milestone value passed preflight and then failed
inside execFileSync after earlier actions had been applied. Preflight now
names the line and key; tests cover a single action and a NUL value placed
after valid actions (zero gh calls, log unchanged).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
sungjunlee added a commit that referenced this pull request Sep 29, 2026
…ny write (#505 review)

A 1 MiB reason or milestone passed preflight and failed with E2BIG at spawn
time after earlier actions had been applied. The cap sits below GitHub's
65,536-character comment limit and Linux's 128 KiB per-argument limit
(a close-duplicate comment joins two capped values). This closes the
"passes preflight, fails deterministically at spawn" class; network,
missing-target, and GitHub-side validation failures stay with the apply
log's resume.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
sungjunlee added a commit that referenced this pull request Sep 29, 2026
…review)

Codex connector review: `reason="a" reason="b"` kept the last value and
trailing text was ignored, so a malformed accepted anchor could still write;
and two same-value milestone or priority anchors (legacy `name=` alias, or
a different reason) produced the same edit twice. Preflight now rejects a
repeated key, unparsed argument text, and any second priority or milestone
assignment for an issue. apply.md lists the preflight rules; the CHANGELOG
line points there instead of enumerating them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
sungjunlee added a commit that referenced this pull request Sep 29, 2026
…er the cap (#505 review)

A 59,999-digit target passed the per-value cap, and joined with a
60,000-byte reason the duplicate comment exceeded GitHub's comment limit.
The target must now be #N with N a positive safe integer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
sungjunlee added a commit that referenced this pull request Sep 29, 2026
…action (#505 review)

Grok round 6 P3s: nothing asserted that a report with every source anchor
and its Apply Checklist copy checked still dedupes and applies each action
once; and the issue-number message now says "positive safe integer",
matching the duplicate-target rule.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
sungjunlee added a commit that referenced this pull request Sep 29, 2026
…iew)

Codex connector: a log written with `name="A"` did not match a rerun of the
same assignment rewritten as `milestone="A"`, so the edit ran again. Action
keys now treat `name=` as `milestone=` (when they do not disagree), so an
alias pair in one report is one action and reruns across the spelling
change are `already-applied`. Other verbs' keys are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
sungjunlee added a commit that referenced this pull request Sep 29, 2026
…line (#505 review)

Codex connector: native Windows limits a command line to about 32,767
UTF-16 characters, so a 40,000-character reason passed the 60,000-byte
cap and failed at spawn after earlier writes. 16,000 UTF-8 bytes keeps
the longest gh command (close-duplicate comment) under that limit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ
sungjunlee added a commit that referenced this pull request Sep 29, 2026
…ints (#506)

The active sprint (epic #504, milestone 28) was local-only, which
`_context.md` warns makes it invisible to delegate worktrees branched from
main. Batch 1 is recorded done (#494, #495 via PR #505); Batch 2 (#496) is
next. The Lightening 2 and 3 blueprints under `sprints/planned/` carry
`status: planned` and are not read as active tracks.


Claude-Session: https://claude.ai/code/session_01X9gZYk7FcYSLAZBBe7JXvZ

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@sungjunlee
sungjunlee deleted the codex/lightening-s1 branch September 29, 2026 15:14
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.

triage-apply: canonical milestone key, GitHub-only guard, validate every accepted action before any write

1 participant