Skip to content

[Spec 1273] Harden reset project resolution: retroactive codex review findings - #1314

Merged
waleedkadous merged 1 commit into
mainfrom
builder/1273-postmerge-hardening
Jul 31, 2026
Merged

[Spec 1273] Harden reset project resolution: retroactive codex review findings#1314
waleedkadous merged 1 commit into
mainfrom
builder/1273-postmerge-hardening

Conversation

@waleedkadous

Copy link
Copy Markdown
Contributor

Summary

#1308 merged on 2-way CMAP because the codex lane was down repo-wide. The architect directed a retroactive advisory review of the merged diff; codex returned REQUEST_CHANGES with real findings. Four are fixed here, two are recorded as pre-existing and deliberately not fixed.

Refs #1273

Fixed

1. A status.yaml stating a different id was overruled by its directory name.
codev/projects/1273-old/ holding id: '999' belongs to 999, whatever the directory is called. The dir-name fallback let a renamed or recycled directory claim a builder, and manufactured false ambiguities beside the real project. The fallback now applies only when the file states no id at all.

2. Weak claims survived when corroboration was impossible.
The code read if (expectedProtocol && mismatch) continue — so a noncanonical builder id, which yields no protocol, let every weak claim through. My own comment two lines above said such a claim "cannot be corroborated and is not trusted". The comment was right; the code did the opposite. Third time in this project a docstring asserted what the code did not do.

3. The numeric issueNumber guard dropped BUGFIX issue identity — a regression I introduced in #1308.
BUGFIX deliberately stores porch ids as bugfix-<N> (spawn.ts:817, "historical, kept untouched"), so /^\d+$/ discarded it whenever the registry row had none. On BUGFIX the issue body is the spec, so the re-orientation silently lost the requirements it exists to carry. issueNumberFromPorchId now accepts a bare number or canonical <word>-<number>, still rejecting ad-hoc task ids.

4. Bare-task detection was a negative inference.
initPorchInWorktree is deliberately non-fatal, so a --task --protocol X builder whose porch init failed also has task text and no porch — and taskText && !porch stripped its real protocol template and handed it raw task text. Replaced with positive evidence: isBareTask, true only when task text is present and the prompt carries no ## Mode: heading. The prompt is written before porch init (spawn.ts:545-548), so a --task --protocol builder's rendered template survives a failed init and is reliable evidence. This also correctly re-scopes the mode default.

Recorded, not fixed

Both are pre-existing spawn properties rather than regressions from this work, and neither is reachable in this repo today:

  • Hyphenated protocol namesbuilder-my-protocol-1273 parses as protocol my via parseAgentName's first-dash split. Needs a custom protocol containing a hyphen; none exists. Fixing it means constraining protocol names at spawn or changing the agent-name encoding.
  • Task short-id uniqueness — 24-bit random, unchecked against committed porch history, so a reused builder-task-xxxx could strong-match a stale project. Probabilistic and spawn-side; belongs with spawn: builder id is lowercased but the worktree/branch name preserves case #1312.

Testing

Suite 4035 → 4041, build clean. Six new regression tests, one per finding plus the two isBareTask directions.

Live re-verified with --dry-run (zero writes): 1273→ASPIR, 1286→ASPIR, task-re_v→TASK.

Note on the review that produced this

Worth recording: codex was this project's most productive reviewer by a wide margin, and #1308 shipped without it purely due to an infrastructure outage. The retroactive pass found a regression that PR had introduced. That is a fairly strong argument for treating a missing CMAP lane as a gap to close rather than a quorum to route around.

…iew of #1308

#1308 merged on 2-way CMAP during the codex outage. Retroactive advisory review
(architect-directed) returned REQUEST_CHANGES with real findings. Four fixed
here; two recorded as pre-existing and not fixed.

1. A status.yaml stating a DIFFERENT id was overruled by its directory name.
   `codev/projects/1273-old/` holding `id: '999'` belongs to 999, whatever the
   directory is called. The dir-name fallback let a renamed or recycled
   directory claim a builder, and manufactured false ambiguities beside the
   real project. The fallback now applies ONLY when the file states no id.

2. Weak claims survived when corroboration was impossible. The code read
   `if (expectedProtocol && mismatch) continue`, so a noncanonical builder id —
   which yields no protocol — let EVERY weak claim through. My own comment two
   lines above said such a claim "cannot be corroborated and is not trusted".
   The comment was right and the code did the opposite; now it rejects.
   (Third time this project that a docstring asserted what the code did not do.)

3. The numeric issueNumber guard dropped BUGFIX issue identity. BUGFIX stores
   porch ids as `bugfix-<N>` deliberately (spawn.ts:817, "historical, kept
   untouched"), so /^\d+$/ discarded it whenever the registry row had none — and
   on BUGFIX the issue body IS the spec, so the re-orientation lost the
   requirements it exists to carry. This was a regression I introduced in
   #1308's own issueNumber fix. New issueNumberFromPorchId accepts a bare number
   or a canonical `<word>-<number>`, still rejecting ad-hoc task ids.

4. Bare-task detection was a negative inference. initPorchInWorktree is
   deliberately non-fatal, so a `--task --protocol X` builder whose porch init
   FAILED also has task text and no porch — and `taskText && !porch` stripped
   its real protocol template and handed it raw task text instead. Replaced
   with positive evidence: `isBareTask` on the resolved context, true only when
   task text is present AND the prompt carries no `## Mode:` heading. The prompt
   is written BEFORE porch init (spawn.ts:545-548), so a --task --protocol
   builder's rendered template survives a failed init and is reliable evidence.
   This also correctly re-scopes the F2 mode default.

RECORDED, NOT FIXED — both pre-existing spawn properties rather than
regressions from this work, and neither is reachable in this repo today:

- Hyphenated protocol names: `builder-my-protocol-1273` parses as protocol `my`
  via parseAgentName's first-dash split. Needs a custom protocol containing a
  hyphen; none exists. Fixing it means constraining protocol names at spawn or
  changing the agent-name encoding — outside a reset fix.
- Task short ids are 24-bit random and not checked against committed porch
  history, so a reused `builder-task-xxxx` could strong-match a stale project.
  Probabilistic and spawn-side; belongs with the id-generation issue (#1312).

Live re-verified: 1273 -> ASPIR, 1286 -> ASPIR, task-re_v -> TASK.
Suite 4035 -> 4041, build clean.
@waleedkadous
waleedkadous merged commit 05b2f76 into main Jul 31, 2026
6 checks passed
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