joshtronic/igor

Watch

feat(ceo): Phase 4 follow-up -- conversational proposals + two-way on proposals + metrics hardening #286

Merged

joshtronic merged 1 commit from feat/ceo-phase4-followup into master

2026-06-30 01:18:56 +00:00

igor commented

2026-06-30 01:11:55 +00:00

Collaborator

CEO Phase 4 follow-up — three fixes the CEO's first live porksicle cycle surfaced (#100/#101).

1. Conversational bodies (directive). The first proposals read like dry feature tickets. The directive now has the CEO open each proposal/question body with its read — what in the numbers/week put it on the desk, the strategic why, the call it's making, in its own voice — then scope/acceptance. Reasoning first, spec second.

2. Two-way on proposals (the real catch). You commented on proposal #101 and unassigned yourself — the same "act on this" gesture the question channel uses — but Path 1 only read question comments, so your feedback would have sat unread. New Path 1b: when the reviewer comments on a ceo-proposal and unassigns without the Agent label, the CEO reconsiders it:

WITHDRAW — reply + close (the board is right)
REVISE — reply + re-file a sharper proposal
HOLD — reply + hand it back to the board (makes its case)

It does not do the proposed work (that stays the Agent-label → grind path). Detection requires unassigned + not Agent-labeled + has a comment, so a greenlight never trips it. One reconsider per tick (after answered-questions); every branch (incl. unparseable) closes or re-assigns so there's no per-tick reconsider loop; a REVISE re-file respects CEO_MAX_OPEN.

3. Metrics-fetch hardening (from PR 2's shadow review): ceo_read_metrics now requires https:// (blocks file://, http://-internal, cloud-metadata) and caps the body (head -c 65536) — an oversize/hostile response truncates past valid JSON and degrades to the existing "unavailable" path.

New: ceo_responded_proposal_numbers, ceo_proposal_thread, ceo_build_reconsider_prompt, ceo_parse_reconsider. Tests cover the reconsider parse (incl. failure modes), the detection, and both hardening guards.

Verified: bash -n + bin/test-ceo.sh (all pass) + check-sync green. Note: built foreground — a fork/API glitch ate two delegated attempts, so I wrote and reviewed it directly (incl. catching + fixing the HOLD-reconsider loop myself before commit). No reviewer.

CEO **Phase 4 follow-up** — three fixes the CEO's first live porksicle cycle surfaced (#100/#101). **1. Conversational bodies (directive).** The first proposals read like dry feature tickets. The directive now has the CEO open each proposal/question body with its **read** — what in the numbers/week put it on the desk, the strategic why, the call it's making, in its own voice — *then* scope/acceptance. Reasoning first, spec second. **2. Two-way on proposals (the real catch).** You commented on proposal **#101** and unassigned yourself — the same "act on this" gesture the question channel uses — but Path 1 only read *question* comments, so your feedback would have sat unread. New **Path 1b**: when the reviewer comments on a `ceo-proposal` and unassigns **without** the `Agent` label, the CEO **reconsiders** it: - **WITHDRAW** — reply + close (the board is right) - **REVISE** — reply + re-file a sharper proposal - **HOLD** — reply + hand it back to the board (makes its case) It does **not** do the proposed work (that stays the `Agent`-label → grind path). Detection requires unassigned + **not** Agent-labeled + has a comment, so a greenlight never trips it. One reconsider per tick (after answered-questions); every branch (incl. unparseable) closes or re-assigns so there's **no per-tick reconsider loop**; a REVISE re-file respects `CEO_MAX_OPEN`. **3. Metrics-fetch hardening** (from PR 2's shadow review): `ceo_read_metrics` now requires `https://` (blocks `file://`, `http://`-internal, cloud-metadata) and caps the body (`head -c 65536`) — an oversize/hostile response truncates past valid JSON and degrades to the existing "unavailable" path. New: `ceo_responded_proposal_numbers`, `ceo_proposal_thread`, `ceo_build_reconsider_prompt`, `ceo_parse_reconsider`. Tests cover the reconsider parse (incl. failure modes), the detection, and both hardening guards. Verified: `bash -n` + `bin/test-ceo.sh` (all pass) + check-sync green. Note: built foreground — a fork/API glitch ate two delegated attempts, so I wrote and reviewed it directly (incl. catching + fixing the HOLD-reconsider loop myself before commit). No reviewer.

igor added 1 commit

2026-06-30 01:11:55 +00:00

feat(ceo): Phase 4 follow-up -- conversational proposals + two-way on proposals + metrics hardening

Lint / check-sync (pull_request) Successful in 5s

Details

Lint / check-sync (push) Successful in 4s

Details

d4dcb5ae35

Three fixes surfaced by the CEO's first live cycle on porksicle (#100/#101).

1. CONVERSATIONAL BODIES (directive). The first proposals read like dry feature
   tickets. The directive now tells the CEO to open each proposal/question body with
   its READ -- what in the numbers/week put it on the desk, the strategic why, the
   call it's making, in its own voice -- THEN the scope/acceptance. Reasoning first,
   spec second.

2. TWO-WAY ON PROPOSALS. Josh commented on proposal #101 and unassigned himself --
   the same 'act on this' gesture the question channel uses -- but Path 1 only read
   question comments, so the feedback would have sat unread. New Path 1b: when the
   reviewer comments on a ceo-proposal and unassigns WITHOUT the Agent label, the CEO
   reconsiders it -- WITHDRAW (reply + close), REVISE (reply + re-file a sharper
   proposal), or HOLD (reply + hand back to the board). NOT doing the work (that
   stays the Agent-label-then-grind path). Detection requires unassigned + not
   Agent-labeled + has a comment, so a greenlight never trips it; one reconsider per
   tick, after answered-questions; WITHDRAW/REVISE/HOLD and the unparseable case all
   exit the responded-set (close or re-assign) so there's no per-tick loop; a REVISE
   re-file respects CEO_MAX_OPEN. New: ceo_responded_proposal_numbers,
   ceo_proposal_thread, ceo_build_reconsider_prompt, ceo_parse_reconsider.

3. METRICS-FETCH HARDENING (from PR2's shadow review). ceo_read_metrics now requires
   https:// (blocks file://, http://internal, cloud-metadata) and caps the body
   (head -c 65536) -- an oversize/hostile response truncates past valid JSON and
   degrades to the existing 'unavailable' path.

Tests: reconsider parse (WITHDRAW/REVISE/HOLD + sentinels + failure modes),
responded-proposal detection, https-rejection + size-cap. bash -n + test-ceo +
check-sync all green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011KXPafSYJPY6XkWHHk1Lfs

igor commented

2026-06-30 01:15:57 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for d4dcb5ae: success

Summary: Adds three CEO Phase-4 follow-ups — conversational proposal/question bodies (directive prose), a new "Path 1b" that reconsiders a proposal the board commented on + handed back (WITHDRAW/REVISE/HOLD), and https-only + size-cap hardening on the metrics fetch. CI is green, the diff is ~200 lines, and the description maps honestly to the diff. My one reason for not approving outright: the new tick.sh state machine (which closes and re-files live issues) has no direct test, and this was a self-built, unreviewed change — I want a human to eyeball the orchestration.

Findings

bin/tick.sh Path 1b (~L1902-1950) — untested orchestration on a live mutation path. The loop-prevention reasoning traces correctly to me: WITHDRAW/REVISE both PATCH state:closed (drops out of the state=open query), and HOLD re-assigns the reviewer (drops out of the index($r) | not filter in ceo_responded_proposal_numbers). So no per-tick reconsider loop, as claimed. But the only tests are on the helpers (ceo_parse_reconsider, ceo_responded_proposal_numbers) — the close/re-file/re-assign flow itself is exercised by nothing. Given it closes and re-files real issues, a human should confirm this path before it runs live.
bin/tick.sh — prompt, raw, attempt are not declared local. The local line only lists responded rnum rparsed rdecision rreply rissue. It's harmless today (every branch hits return 0), but these leak into the enclosing scope and are a foot-gun for future edits; add them to the local declaration.
bin/tick.sh REVISE-at-cap (~L1932-1940) — proposal silently dropped. When ceo_open_items_count >= CEO_MAX_OPEN, the old proposal is closed with a "tightening scope" reply comment, but no revision is re-filed. The board sees a closed issue promising a revision that never appears (it's logged, but not surfaced to the reviewer). Minor UX wrinkle, not a blocker.
Scope: the metrics hardening (lib/ceo.sh ceo_read_metrics) is a separate, orthogonal security fix ("from PR 2's shadow review") bundled with the proposal-reconsider feature. Mild scope creep — defensible as one "follow-up," but it's not the same concern as #1/#2.
Unverifiable from the diff (not blocking): _ceo_trim_blanks, _ceo_parse_issues, ceo_file_proposal, ceo_open_items_count, claude_call, _fj, CEO_PROPOSAL_MARKER, CEO_MAX_OPEN, forgejo_assign are all pre-existing and not shown; green CI + the new unit tests give reasonable confidence they behave.

Positives

The metrics hardening is a genuine improvement: https://* allowlist blocks file:///http://-internal/cloud-metadata, and head -c 65536 caps a hostile/oversize body so it degrades to the existing "unavailable" path. Both guards are tested.
ceo_parse_reconsider keeps JSON harness-built (never model-written), validates DECISION + ===REPLY===, and has failure-mode tests (missing decision, missing sentinel, whitespace-only reply).
Detection requires unassigned + not-Agent-labeled + has-comment + marker, so a greenlight or a bare unassign can't trip it — the test table (#20–#24) covers exactly those discriminators.
Description checklist is honest; no CI-config changes; under the 400-line bar.

Recommendation: Have a human confirm the Path 1b live-mutation flow and decide whether the REVISE-at-cap silent-drop is acceptable; consider adding the missing locals. Nothing here clearly blocks, but the untested issue-closing/re-filing logic on an unreviewed change is worth a human glance before merge.

_{Independent review by the harness on claude-opus-4-8. The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `d4dcb5ae`: **success** **Summary:** Adds three CEO Phase-4 follow-ups — conversational proposal/question bodies (directive prose), a new "Path 1b" that reconsiders a proposal the board commented on + handed back (WITHDRAW/REVISE/HOLD), and https-only + size-cap hardening on the metrics fetch. CI is green, the diff is ~200 lines, and the description maps honestly to the diff. My one reason for not approving outright: the new tick.sh state machine (which closes and re-files live issues) has no direct test, and this was a self-built, unreviewed change — I want a human to eyeball the orchestration. **Findings** - **`bin/tick.sh` Path 1b (~L1902-1950) — untested orchestration on a live mutation path.** The loop-prevention reasoning traces correctly to me: WITHDRAW/REVISE both `PATCH state:closed` (drops out of the `state=open` query), and HOLD re-assigns the reviewer (drops out of the `index($r) | not` filter in `ceo_responded_proposal_numbers`). So no per-tick reconsider loop, as claimed. But the *only* tests are on the helpers (`ceo_parse_reconsider`, `ceo_responded_proposal_numbers`) — the close/re-file/re-assign flow itself is exercised by nothing. Given it closes and re-files real issues, a human should confirm this path before it runs live. - **`bin/tick.sh` — `prompt`, `raw`, `attempt` are not declared `local`.** The `local` line only lists `responded rnum rparsed rdecision rreply rissue`. It's harmless today (every branch hits `return 0`), but these leak into the enclosing scope and are a foot-gun for future edits; add them to the `local` declaration. - **`bin/tick.sh` REVISE-at-cap (~L1932-1940) — proposal silently dropped.** When `ceo_open_items_count >= CEO_MAX_OPEN`, the old proposal is closed *with* a "tightening scope" reply comment, but no revision is re-filed. The board sees a closed issue promising a revision that never appears (it's logged, but not surfaced to the reviewer). Minor UX wrinkle, not a blocker. - **Scope:** the metrics hardening (`lib/ceo.sh` `ceo_read_metrics`) is a separate, orthogonal security fix ("from PR 2's shadow review") bundled with the proposal-reconsider feature. Mild scope creep — defensible as one "follow-up," but it's not the same concern as #1/#2. - **Unverifiable from the diff (not blocking):** `_ceo_trim_blanks`, `_ceo_parse_issues`, `ceo_file_proposal`, `ceo_open_items_count`, `claude_call`, `_fj`, `CEO_PROPOSAL_MARKER`, `CEO_MAX_OPEN`, `forgejo_assign` are all pre-existing and not shown; green CI + the new unit tests give reasonable confidence they behave. **Positives** - The metrics hardening is a genuine improvement: `https://*` allowlist blocks `file://`/`http://`-internal/cloud-metadata, and `head -c 65536` caps a hostile/oversize body so it degrades to the existing "unavailable" path. Both guards are tested. - `ceo_parse_reconsider` keeps JSON harness-built (never model-written), validates DECISION + ===REPLY===, and has failure-mode tests (missing decision, missing sentinel, whitespace-only reply). - Detection requires unassigned + not-Agent-labeled + has-comment + marker, so a greenlight or a bare unassign can't trip it — the test table (#20–#24) covers exactly those discriminators. - Description checklist is honest; no CI-config changes; under the 400-line bar. **Recommendation:** Have a human confirm the Path 1b live-mutation flow and decide whether the REVISE-at-cap silent-drop is acceptable; consider adding the missing `local`s. Nothing here clearly blocks, but the untested issue-closing/re-filing logic on an unreviewed change is worth a human glance before merge. --- <sub>Independent review by the harness on `claude-opus-4-8`. The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-06-30 01:15:57 +00:00

1 minute 48 seconds

igor requested review from joshtronic

2026-06-30 01:15:58 +00:00

igor force-pushed feat/ceo-phase4-followup from d4dcb5ae35

Lint / check-sync (pull_request) Successful in 5s

Details

Lint / check-sync (push) Successful in 4s

Details

to 3d6b2f6cf6

Lint / check-sync (push) Successful in 4s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

2026-06-30 01:16:47 +00:00

Compare

joshtronic approved these changes

2026-06-30 01:18:19 +00:00

joshtronic merged commit 1ad967415b into master

2026-06-30 01:18:56 +00:00

joshtronic deleted branch feat/ceo-phase4-followup

2026-06-30 01:18:57 +00:00

joshtronic referenced this pull request from a commit

2026-06-30 01:18:58 +00:00

Merge pull request 'feat(ceo): Phase 4 follow-up -- conversational proposals + two-way on proposals + metrics hardening' (#286) from feat/ceo-phase4-followup into master

igor commented

2026-06-30 01:19:47 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 3d6b2f6c: success

Summary: CEO Phase 4 follow-up — directive tweaks for conversational proposal bodies, a new "reconsider a handed-back proposal" path (Path 1b in tick.sh + four helpers in lib/ceo.sh), and SSRF/size hardening of the metrics fetch. CI is green, scope is coherent, and the description's claims all map to real diff. No blocking defect, but a couple of reservations I'd want a human to weigh before merge.

Checklist / honesty check (all verified):

Conversational directive bullets — present in bin/lib/ceo-digest-directive.md. ✓
Path 1b reconsider flow — present in bin/tick.sh; helpers ceo_responded_proposal_numbers, ceo_proposal_thread, ceo_build_reconsider_prompt, ceo_parse_reconsider all present in lib/ceo.sh. ✓
Metrics hardening (https-only + head -c 65536) — present in ceo_read_metrics. ✓
Tests claimed (parse incl. failure modes, detection, both hardening guards) — all present in bin/test-ceo.sh. ✓

Positives:

ceo_read_metrics hardening is genuinely good defense: the https://* case-guard blocks file:///http://-internal/metadata, and since curl has no -L it won't follow a redirect into those either. The size cap degrades cleanly to the existing "unavailable" path (tested).
Loop-prevention reasoning is sound for the HOLD and unparseable branches — re-assigning the reviewer drops the issue out of state=open/unassigned filtering, so it can't reconsider-loop. The detection (unassigned + not Agent-labeled + comments>0) correctly avoids tripping on freshly-filed (still-assigned) or greenlit (Agent-labeled) proposals; the 5-case detection test confirms each exclusion.

Reservations (none blocking, but worth a human glance):

WITHDRAW/REVISE loop on persistent close failure (bin/tick.sh, the case block): both paths post a comment then PATCH ... "closed" with || true, relying on the close to drop the issue out of the state=open set. If the close API call persistently fails (perms, etc.), the issue stays open + unassigned + now has the CEO's reply comment → it re-enters the responded-set next tick and gets re-withdrawn, spamming a new comment each tick. The HOLD/unparseable paths were deliberately guarded against this via re-assign; WITHDRAW/REVISE were not. Transient failures self-heal, so this is an edge case — but it's the one spot where the "no per-tick reconsider loop" guarantee in the description isn't actually enforced.
Core orchestration is untested. The new tests cover the four helpers in isolation (good coverage on ceo_parse_reconsider failure modes), but the Path 1b branching itself — the WITHDRAW/REVISE/HOLD dispatch, the re-file CEO_MAX_OPEN gate, the close-then-count ordering — has no test. That's the most state-mutating, error-prone part of the change. It reads correctly on inspection (close happens before the ceo_open_items_count check, so the freed slot is counted; comment posts before the case), but I'm verifying by eye, not by test.
Minor: prompt, raw, attempt in the Path 1b block are not in the local declaration (local responded rnum rparsed rdecision rreply rissue). Likely benign reuse of locals already declared earlier in do_ceo_tick, but worth confirming they don't leak.

Couldn't verify from the diff: _ceo_parse_issues, _ceo_trim_blanks, ceo_file_proposal, forgejo_assign, claude_call signature, and the CEO_PROPOSAL_MARKER/CEO_MAX_OPEN/FORGEJO_REVIEWER definitions all live outside this diff; I'm assuming existing behavior. The PR also notes it was built foreground with no reviewer, which is the main reason I'm routing this to a human rather than approving.

_{Independent review by the harness on claude-opus-4-8. The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `3d6b2f6c`: **success** **Summary:** CEO Phase 4 follow-up — directive tweaks for conversational proposal bodies, a new "reconsider a handed-back proposal" path (Path 1b in `tick.sh` + four helpers in `lib/ceo.sh`), and SSRF/size hardening of the metrics fetch. CI is green, scope is coherent, and the description's claims all map to real diff. No blocking defect, but a couple of reservations I'd want a human to weigh before merge. **Checklist / honesty check (all verified):** - Conversational directive bullets — present in `bin/lib/ceo-digest-directive.md`. ✓ - Path 1b reconsider flow — present in `bin/tick.sh`; helpers `ceo_responded_proposal_numbers`, `ceo_proposal_thread`, `ceo_build_reconsider_prompt`, `ceo_parse_reconsider` all present in `lib/ceo.sh`. ✓ - Metrics hardening (https-only + `head -c 65536`) — present in `ceo_read_metrics`. ✓ - Tests claimed (parse incl. failure modes, detection, both hardening guards) — all present in `bin/test-ceo.sh`. ✓ **Positives:** - `ceo_read_metrics` hardening is genuinely good defense: the `https://*` case-guard blocks `file://`/`http://`-internal/metadata, and since `curl` has no `-L` it won't follow a redirect into those either. The size cap degrades cleanly to the existing "unavailable" path (tested). - Loop-prevention reasoning is sound for the HOLD and unparseable branches — re-assigning the reviewer drops the issue out of `state=open`/unassigned filtering, so it can't reconsider-loop. The detection (`unassigned + not Agent-labeled + comments>0`) correctly avoids tripping on freshly-filed (still-assigned) or greenlit (Agent-labeled) proposals; the 5-case detection test confirms each exclusion. **Reservations (none blocking, but worth a human glance):** - **WITHDRAW/REVISE loop on persistent close failure** (`bin/tick.sh`, the `case` block): both paths post a comment then `PATCH ... "closed"` with `|| true`, relying on the close to drop the issue out of the `state=open` set. If the close API call persistently fails (perms, etc.), the issue stays open + unassigned + now has the CEO's reply comment → it re-enters the responded-set next tick and gets re-withdrawn, spamming a new comment each tick. The HOLD/unparseable paths were deliberately guarded against this via re-assign; WITHDRAW/REVISE were not. Transient failures self-heal, so this is an edge case — but it's the one spot where the "no per-tick reconsider loop" guarantee in the description isn't actually enforced. - **Core orchestration is untested.** The new tests cover the four helpers in isolation (good coverage on `ceo_parse_reconsider` failure modes), but the Path 1b branching itself — the WITHDRAW/REVISE/HOLD dispatch, the re-file `CEO_MAX_OPEN` gate, the close-then-count ordering — has no test. That's the most state-mutating, error-prone part of the change. It reads correctly on inspection (close happens before the `ceo_open_items_count` check, so the freed slot is counted; comment posts before the case), but I'm verifying by eye, not by test. - **Minor:** `prompt`, `raw`, `attempt` in the Path 1b block are not in the `local` declaration (`local responded rnum rparsed rdecision rreply rissue`). Likely benign reuse of locals already declared earlier in `do_ceo_tick`, but worth confirming they don't leak. **Couldn't verify from the diff:** `_ceo_parse_issues`, `_ceo_trim_blanks`, `ceo_file_proposal`, `forgejo_assign`, `claude_call` signature, and the `CEO_PROPOSAL_MARKER`/`CEO_MAX_OPEN`/`FORGEJO_REVIEWER` definitions all live outside this diff; I'm assuming existing behavior. The PR also notes it was built foreground with no reviewer, which is the main reason I'm routing this to a human rather than approving. --- <sub>Independent review by the harness on `claude-opus-4-8`. The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-06-30 01:19:48 +00:00

1 minute 50 seconds