feat(bin): local ChatGPT return transport, park filter, and packet merge authority - #4344
brentwarnes-repo wants to merge 3 commits into
Conversation
…uthority Implement three local controls so the captain's required First Mate behavior no longer depends on the parked upstream PR kunchenguid#4315: - bin/fm-chatgpt-return.sh: atomically write a ChatGPT-bound captain return, diagnosed and fixed independently of the parked branch's copy (dangling-symlink canonicalization via realpath -m instead of a per-component Cwd::realpath that silently fell back to the literal path; PR-identity dedup by repo+number instead of bare number, so two different repos' PR kunchenguid#6 no longer collapse into one). - bin/fm-captain-hold.sh hold --park: records parked/standby execution state distinct from a live captain call, excluded from the OPEN DECISIONS drain (bin/fm-classify-lib.sh) via a state/<id>.parked marker, and fixes the parent-decision resolution defect by resolving a task's dangling live hold when it is parked instead of leaving it open. - bin/fm-packet.sh: records and checks bounded packet-scoped merge authority (open/grant/check/close) as pure recording/checking mechanics, not wired into an autonomous merge path. PR kunchenguid#4315 and its preserved branch remain untouched throughout.
…om OPEN DECISIONS surfacing
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f7155542c
ℹ️ 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".
| record=$(printf '%s\n' "$record" | sed \ | ||
| -e "s/^merge_authority=.*/merge_authority=yes/" \ | ||
| -e "s/^granted=.*/granted=$(now_stamp)/") | ||
| write_record_atomic "$slug" "$record" |
There was a problem hiding this comment.
When grant and close run concurrently, each reads the old open record and later replaces the entire file without a shared lock or compare-and-swap. If grant writes last, it restores status=open, clears the close timestamp, and leaves merge_authority=yes, so check authorizes merges after the packet was closed. Serialize every read-modify-write operation for a packet so closure cannot be overwritten by a stale grant.
AGENTS.md reference: AGENTS.md:L357-L358
Useful? React with 👍 / 👎.
| if [ "$existing_hold_kind" = captain ] && [ "$existing_held" = yes ]; then | ||
| occurrence=$(( $(resolution_record_count "$(show_field "$show" body)") + 1 )) | ||
| publish_parent_hold "$id" "$occurrence" resolved parked | ||
| fi |
There was a problem hiding this comment.
When parking an existing live hold in a secondmate home and the parent-channel write fails, publish_parent_hold only reports the failure while the backlog still changes to hold_kind=parked. Retrying hold --park after repairing the route then sees existing_hold_kind=parked, skips this conditional, and never republishes the missing resolution, leaving the parent's prior needs-decision open indefinitely. Preserve enough durable transition state to retry the resolution until it reaches the parent. .agents/skills/captain-hold-lifecycle/SKILL.mdL58-L60
Useful? React with 👍 / 👎.
| if [ -z "$ids" ]; then | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
When a return claims a PR count but lists PRs without numbered identities, collect_pr_ids returns empty and this branch accepts the claim without comparing it to the bullet count. For example, Three PRs landed: followed by two title-only bullets passes verification, so the advertised count/list QA still permits an inaccurate captain-facing return. Treat an empty ID set as zero or count the associated list entries before accepting the claim.
AGENTS.md reference: AGENTS.md:L515-L516
Useful? React with 👍 / 👎.
| if [ -n "${FM_TASK_ID:-}" ]; then | ||
| default_canonical=$(canonical_path "$DEFAULT_RETURN_PATH") \ | ||
| || fail "could not canonicalize the live return path" | ||
| if [ "$dest" = "$default_canonical" ]; then | ||
| fail "a task worker must not write the live ChatGPT return transport" |
There was a problem hiding this comment.
When a task worker invokes this script through an environment-clearing wrapper or otherwise lacks FM_TASK_ID, the only worker-specific guard is skipped and the process can overwrite the live ChatGPT transport. Because the script is intended to categorically refuse task workers, identify worker scope from non-caller-controlled home/worktree metadata rather than relying solely on an optional environment variable.
AGENTS.md reference: AGENTS.md:L515-L516
Useful? React with 👍 / 👎.
…d#4344 Four ordinary QA/implementation fixes to the local ChatGPT return transport, park filter, and packet merge authority controls, each with a dedicated regression test verified to fail before the fix and pass after it: - bin/fm-packet.sh: serialize open/grant/close on a per-slug lock (fm-wake-lib.sh's fm_lock_acquire_wait) so a concurrent grant and close can no longer race each other's read-modify-write and resurrect merge authority on a closed packet. - bin/fm-captain-hold.sh: a park whose parent-channel publish fails now leaves a durable state/<id>.park-pending-resolve marker, so a later hold --park retries the deferred resolution instead of losing it once existing_hold_kind no longer reads "captain". - bin/fm-chatgpt-return.sh: the PR-count/list QA now falls back to counting a claim's own list bullets when no numbered PR identity is present, instead of accepting an unchecked claim on no evidence. - bin/fm-chatgpt-return.sh: the live-path guard now also refuses whenever the script's own root is not a genuine primary checkout (fm_primary_scope_matches), so an environment-clearing wrapper that merely unsets FM_TASK_ID can no longer make a task worker pass as the primary.
|
Closed as misaligned with VISION.md — this change takes the project outside the documented scope/contract. — Kun's Firstmate |
1 participant
Intent
Make this Captain's required First Mate behavior independent of parked upstream PR #4315 (kunchenguid/firstmate). PR #4315 itself remains PARKED and must not be resumed, modified, pushed, validated, or chased upstream; its preserved branch (fm/followon-decision-filter at /home/leah/.treehouse/firstmate-7bab20/5/firstmate, head b9d8b97) may be used only as implementation/review evidence or source material.
Implement locally, as three independently testable local controls:
A. Automatic ChatGPT return transport: whenever the primary First Mate produces a return explicitly intended for ChatGPT, the complete return must be atomically written to ~/inbox/FIRST_MATE_TO_CHATGPT.md BEFORE presentation to the Captain. The primary First Mate is the consolidating writer. Do not dump routine internal agent chatter into the transport file. The parked PR's implementation has two known unresolved defects that must be diagnosed and fixed, not copied forward: (1) a dangling-symlink path canonicalization issue, and (2) a repo-identity dedup issue. Test with a small real ChatGPT-bound return.
B. DO IT / DECIDE / REVIEW: implement a local Captain-facing filter with three categories - DO IT (no genuine human judgment remains; execute/resolved AI-side), DECIDE (consequential unresolved human judgment genuinely remains), REVIEW (finished/review-ready artifact needs human judgment/taste/approval). PARK/HOLD is execution state and must NOT automatically surface as a current Captain decision. The parked PR's known defect here - parent-decision resolution behaves incorrectly when parking a live call - must be resolved before this control is considered complete.
C. Packet-scoped merge authority: implement/support the bounded model already proven operationally - an explicitly authorized execution packet may grant packet-scoped merge authority, under which conforming internal implementation PRs may merge without another per-PR Captain word only when: scope remains inside the packet; tests/checks pass; required review completes; substantive findings are repaired/dispositioned; verification passes; no material scope expansion occurs; no reserved human gate appears. This authority expires when the packet closes, does not enable YOLO, and is not global standing merge authority.
D. Ownership: these are Captain/local operating requirements. The Captain's local system must NOT depend on kunchenguid/firstmate accepting an upstream contribution. Use the smallest existing First Mate-local configuration/governance/overlay mechanism that fits; do not invent unnecessary architecture.
Done condition: all three local controls (A, B, C) are independently tested. PR #4315 remains untouched/PARKED throughout.
What Changed
bin/fm-chatgpt-return.sh, which atomically writes the complete ChatGPT-bound return to~/inbox/FIRST_MATE_TO_CHATGPT.mdbefore Captain presentation, fixing dangling-symlink path canonicalization and repo-identity dedup issues.bin/fm-packet.shimplementing packet-scoped merge authority (scope, tests/checks, review, findings disposition, verification, no scope expansion, no reserved gate), expiring when the packet closes.bin/fm-captain-hold.shandbin/fm-classify-lib.shso PARK/HOLD execution state is filtered out of DO IT / DECIDE / REVIEW surfacing and no longer misreports parent-decision resolution when parking a live call.AGENTS.md,docs/architecture.md,docs/captain-hold-lifecycle.md,docs/scripts.md, and theask-user-authority/captain-hold-lifecycleskills to document the three local controls, plus added test coverage intests/fm-chatgpt-return.test.sh,tests/fm-captain-hold-park.test.sh, andtests/fm-packet.test.sh.🤖 Generated with Claude Code
Risk Assessment
Testing
All three independently-testable local controls (A: ChatGPT return transport, B: DO IT/DECIDE/REVIEW park filter, C: packet-scoped merge authority) have dedicated test suites in this diff and all pass, including regression tests for the two named upstream defects (dangling-symlink canonicalization, cross-repo PR-identity dedup) and the park/parent-decision-resolution defect; a manual end-to-end CLI run additionally confirms the ChatGPT transport file is produced and formatted as intended. No touch to the parked PR #4315 branch was made.
Evidence: Manual CLI transcript: fm-chatgpt-return.sh write end-to-end
Source: Manual CLI transcript: fm-chatgpt-return.sh write end-to-end
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-packet.sh:15- Control C's own header states it 'records and checks the packet only - it does not itself drive or authorize a merge' and 'no such wiring is added by this script' — bin/fm-pr-merge.sh is untouched by this branch and never consultsfm-packet.sh check. User intent C requires 'conforming internal implementation PRs may merge without another per-PR Captain word only when [conditions]'; fm-pr-merge.sh's own comments show captain-hold state is the actual mechanical gate that blocks a merge until ananswer --release, and that gate is not bypassed for a granted packet anywhere in this change. As shipped, opening/granting a packet changes no actual merge behavior — it only lets code that doesn't exist yet consult it. tests/fm-packet.test.sh (and AGENTS.md's own text) only exercises the record/check lifecycle, not an actual autonomous merge. This may be an intentional MVP scoping consistent with intent's 'implement/support' wording and directive D against inventing unnecessary architecture, but it should be confirmed with the user whether C is meant to be considered complete without the fm-pr-merge.sh wiring, since 'Done condition' calls all three controls independently tested and C's tested surface never actually merges anything without a captain word.bin/fm-packet.sh:137- open/grant/close/check all do read-then-write of the packet record with no lock (unlike fm-captain-hold.sh's acquire_task_control_lock), so two concurrent invocations (e.g. a duplicate grant vs. close) can race and clobber each other's write. Low likelihood given single-operator usage, but worth a follow-up if packets are ever driven concurrently.✅ **Test** - passed
✅ No issues found.
bash tests/fm-chatgpt-return.test.sh (10/10 pass, includes dangling-symlink and cross-repo dedup regression tests)bash tests/fm-packet.test.sh (5/5 pass, packet-scoped merge authority lifecycle)bash tests/fm-captain-hold-park.test.sh (5/5 pass, includes parent-decision resolution on park regression test)Manual CLI transcript: bin/fm-chatgpt-return.sh write against a scratch HOME, verifying the assembled ~/inbox/FIRST_MATE_TO_CHATGPT.md content end-to-endgit status --porcelain (worktree clean after testing)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.