feat(claude-ops): script deterministic lane-cycle mechanics (#538)#803
Conversation
Two /loop lane mechanics that need no reasoning are otherwise hand-assembled every session. Extract them into scripts the lane prompt references, making the output deterministic and testable and shrinking the prompt. - machine-behavior.sh: emits the MACHINE-BEHAVIOR block (gh identity, clone path, worktree inventory, INSTALLED plugin versions) as a verbatim-printable text block. Facts only — "deviations from standing rules" stays a model judgment, not a scripted field. Installed versions are the honest number for a running lane (they can lag repo HEAD mid-session per context/refresh.md). - telemetry-upsert.sh: maintains ONE marker-identified telemetry comment on a tracking issue, editing it in place via a machine-detectable sentinel found across all comments (paginated) — never posting a second. Interim home of the #502 telemetry contract. Both follow lane-launcher.sh conventions (jq CRLF wrapper, --help header as the full contract, explicit exit codes) and ship a sibling .test.sh with PATH-stubbed gh/git and JSON fixtures (no network). Documented in the lanes SKILL.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
|
Claude finished @kyle-sexton's task in 3m 46s —— View job Code Review
SummaryOverall this is a solid PR — determinism contract is well-reasoned, graceful degradation throughout, pagination bug is correctly identified and fixed, marker charset validation is tight, and the hermetic test harness is thorough. One correctness bug in Findings1. Bug — clone-path is
|
|
Claude finished @kyle-sexton's task in 4m 10s —— View job Security Review — PR #803Tasks:
Reviewed the 7 changed files. Security-only findings below; no style or coverage commentary. IMPORTANT — Unvalidated
|
…refs CI on #803 flagged two mechanical gates: - exec-bit: the four new lane scripts carried shebangs but non-executable git modes; set 100755 in the index. - comment-hygiene: two header comments cited issue numbers directly (not the sanctioned TODO(#issue) form). Reworded to describe the why/constraint without the numeric back-reference, and changed a sample-output line to a placeholder. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
|
Claude finished @kyle-sexton's task in 2m 41s —— View job Security Review — PR #803
Two security findings in IMPORTANT —
|
|
Claude finished @kyle-sexton's task in 2m 7s —— View job Re-review after
|
kyle-sexton
added
the
area: security
…worktree bug
Review of the lane scripts surfaced two IMPORTANT security findings and one
correctness bug, all fixed on-branch.
Security (telemetry-upsert.sh is prompt-driven, so its inputs are attacker-
reachable):
- Validate --repo as owner/repo BEFORE interpolating it into any gh api URL
path. An unvalidated value with `..` segments could otherwise redirect the
GET/PATCH/POST to a different repo.
- Constrain a real --body-file to resolve under the safe dir (--body-dir, else
$CLAUDE_PLUGIN_DATA), reject a symlink leaf, and cap the body at 64 KiB.
Without this, a prompt-injected --body-file pointing at a secret (an ssh key,
a token store, a .env) would be cat'd and posted as a public comment. Pipe an
in-memory body via `-` (stdin), which is exempt from the path check.
Correctness:
- machine-behavior.sh emitted "." as the clone path from the main worktree:
`rev-parse --git-common-dir` returns a relative `.git` there, and
dirname(".git") is ".". Anchor a relative common-dir to the repo root first.
Also fixes a vacuous seen_api guard in the telemetry-upsert test stub. New test
cases cover each fix (traversal --repo, out-of-safe-dir body, symlink leaf,
oversized body, no-safe-dir fail-closed, relative git-common-dir clone-path);
the body-file contract is documented in SKILL.md and the CHANGELOG entry.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
|
Claude finished @kyle-sexton's task in 3m 44s —— View job Security Review — PR #803 (re-review after
|
|
Claude finished @kyle-sexton's task in 3m 32s —— View job Re-review after
|
|
Post-green review classification (work lane) — final round:
Both IMPORTANT/DEFER-FORBIDDEN findings were fixed on-branch before this PR went further, per rule 8's carve-out — never deferred to a follow-up issue. No inline review threads exist on this PR (GraphQL query returned empty) — nothing to resolve. Handing off to the babysit lane for ongoing watch. |
|
Claude finished @kyle-sexton's task in 4m 19s —— View job Security Review — PR #803 (fresh pass at HEAD
|
|
Claude finished @kyle-sexton's task in 3m 56s —— View job PR #803 Review
|
![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: a75fd8a8b5
ℹ️ 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".
…k boundary A space-form option passed as the final argument (e.g. `--issue` with no value) left `shift 2` unable to consume it, spinning the parse loop forever instead of returning exit 3; each value option now checks for its argument first. The marker-fallback used a plain substring match, so a shorter marker (`lane:triage`) would adopt and overwrite a longer lane's comment (`lane:triage-old`). It now boundary-matches the marker as a whole token. New test cases cover both: a trailing value-less option exiting 3 without hanging, and a prefix-collision marker creating its own comment rather than PATCHing the longer lane's. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
|
Claude finished @kyle-sexton's task in 3m 12s —— View job Security Review — PR #803 (HEAD
|
|
Claude finished @kyle-sexton's task in 2m 16s —— View job Review — PR #803 (HEAD
|
|
Classification — genuinely-open findings from the two fresh-at-HEAD reviews (security #803 comment 5031262206, code review 5031262309). The two codex P2 inline findings were fixed in
No fix pushed for these three — deferred, not INCORRECT. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7552a1a6e3
ℹ️ 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".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @kyle-sexton's task in 2m 52s —— View job PR #803 Re-review after merge-of-main (
|
| Finding | Status |
|---|---|
clone-path = . from main worktree (machine-behavior.sh L162–170) |
FIXED — normalization block anchors relative common_dir to $REPO; companion test asserts absolute path on STUB_COMMON_DIR=".git" |
--repo URL path injection (telemetry-upsert.sh L252) |
FIXED — ^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$ guard after both explicit and auto-detected paths; traversal test asserts exit 3 |
--body-file arbitrary path / credential exfiltration (telemetry-upsert.sh L182–225) |
FIXED — symlink leaf rejection + pwd -P ancestor canonicalization + 64 KiB cap + fail-closed on missing safe dir; coverage spans all bypass classes |
seen_api guard vacuously true (test stub) |
FIXED — seen_api=0 (integer) with ((seen_api)) arithmetic check |
Fallback contains($m) prefix collision — lane:triage adopts lane:triage-old |
FIXED — boundary regex with Oniguruma lookarounds; . escaped to \.; regression test |
| Trailing space-form option spins parse loop | FIXED — [[ $# -ge 2 ]] guard on all five space-form options; timeout-guarded test asserts exit 3 |
Deferred items — status unchanged (non-blocking)
1. jq() wrapper defined before command -v jq prereq guard (both scripts)
machine-behavior.sh L57 before L63, telemetry-upsert.sh L69 before L73
In bash, command -v jq finds shell functions, so the guard passes even when no jq binary is installed. Subsequent command jq calls inside the wrapper then fail silently: in telemetry-upsert.sh the || comments='[]' fallback fires and the script POSTs a duplicate comment; in machine-behavior.sh individual fields degrade to empty instead of the documented exit 4. The fix is either type -P jq (binary-only lookup) or reordering the prereq check above the wrapper definition. Classified P2 / non-blocking in prior rounds; tracked in #812.
2. case "$body_canon" in "$safe_canon"/* — glob metacharacters in $safe_canon
Glob metacharacters in a bash case pattern are expanded even inside double quotes. A $CLAUDE_PLUGIN_DATA path containing [, ?, or * would widen the accepted prefix. An actor who controls CLAUDE_PLUGIN_DATA or --body-dir already has --body-dir / as a stronger bypass — no new exploitation path. One-line fix: [[ "$body_canon" == "$safe_canon"/* ]]. Deferred; accepted by both prior security reviewers.
3. --repo regex admits foo/.. as a single-segment value
[A-Za-z0-9._-] includes ., so foo/.. passes validation. The resulting URL repos/foo/../issues/N/comments normalizes to a non-existent endpoint (404). Multi-segment traversal (foo/bar/../../org2/target) is correctly rejected by the single-slash constraint. Unexploitable; deferred.
4. gh api user called twice in gh_identity() — micro-inefficiency
Two round-trips to the same endpoint for login and id. No correctness impact; deferred.
No new findings
The merge brought in main-branch content (education, playwright, source-control, skill-quality plugins) with no overlap on the 7 changed files. No conflicts, no behavioral changes to the scripts or tests.
Verdict
All IMPORTANT and correctness blockers remain fixed at HEAD. Deferred items are correctly classified as non-blocking. Ready to merge.
|
Claude finished @kyle-sexton's task in 4m 2s —— View job Security Review — PR #803 (HEAD
|
| # | Finding | Location | Severity | Status |
|---|---|---|---|---|
| 1 | jq() function is defined before command -v jq prereq check; the guard resolves the function, not the binary, so the check passes on a host with no jq binary. Downstream jq calls then fail silently inside $(…), producing empty/default values rather than exit 4. In telemetry-upsert.sh this causes a duplicate comment POST instead of editing the existing one — the "one comment, edit in place" invariant is violated. |
machine-behavior.sh L57/L63, telemetry-upsert.sh L69/L73 |
P2 (deferred) | Tracked in #812. Fix: move the prereq check above the jq() wrapper, or use type -P jq which skips shell functions. |
| 2 | case "$body_canon" in "$safe_canon"/* interprets glob metacharacters inside $safe_canon as pattern syntax (bash case expands [, ?, * in quoted variable expansions). An actor controlling CLAUDE_PLUGIN_DATA/--body-dir already has --body-dir / as a stronger bypass. |
telemetry-upsert.sh L218–L224 |
SUGGESTION (deferred) | Replace case with [[ "$body_canon" == "$safe_canon"/* ]] to treat $safe_canon as a literal. |
| 3 | --repo regex [A-Za-z0-9._-]+/[A-Za-z0-9._-]+$ allows . as a single path component (e.g. foo/..), but the resulting URL repos/foo/../issues/N/comments normalizes to a non-existent endpoint (404, not a cross-repo redirect). The multi-segment traversal attack is correctly blocked by the single-slash constraint. |
telemetry-upsert.sh L252 |
SUGGESTION (deferred) | Unexploitable in stated threat model. |
Fresh observations (no prior classification)
SUGGESTION — stdin body is buffered in full before the size cap is applied
Severity: SUGGESTION | Confidence: PLAUSIBLE
if [[ "$BODY_FILE" == "-" ]]; then
body_text="$(cat)"The $(cat) command substitution reads stdin to completion and holds it in memory before the 64 KiB cap (L229–L233) is checked. The file-path path correctly limits what can be read (safe dir + size cap), but the stdin path has no read-time bound. A prompt-injected lane constructing cat /dev/zero | telemetry-upsert.sh … --body-file - would buffer into process memory indefinitely before the cap fires.
Practical scope: The attacker must control both the stdin source and the --body-file - argument — in the prompt-injection threat model that means the AI model is piping its own output, so the attacker needs fairly deep prompt control. OS process memory limits also constrain the blast radius. This is DoS-class, not credential-exfiltration, and recoverable. Noting for awareness.
Fix option: Read up to MAX_BODY_BYTES + 1 bytes and reject early:
body_text="$(dd bs=1 count=$((MAX_BODY_BYTES + 1)) 2>/dev/null)"(Or head -c if portability is the concern — head -c $((MAX_BODY_BYTES + 1)) is POSIX on Linux/macOS.)
What's well done
- Marker boundary fix (
telemetry-upsert.shL285–L291) —gsub("[.]"; "\\.")correctly escapes only.from the marker charset before building the Oniguruma regex; no other chars in[A-Za-z0-9:._-]have special meaning outside a character class. The lookbehind and lookahead are fixed-width (single character class), so no ReDoS risk. target_idpath —.id // emptyin jq returns a JSON integer type rendered as a digit string by-r; even if an API response were somehow crafted, the PATCH URL can only be poisoned with digits, producing a 404 not a redirect.-f body="$new_body"in the PATCH and POST calls passes the comment body as form data, not a shell token; shell metacharacters in$new_bodyare not further interpreted.- Fail-closed on missing safe dir — exit 4 when neither
--body-dirnor$CLAUDE_PLUGIN_DATAis set; the script cannot be called in an unconstrained state by accident.
Verdict: No new CRITICAL or IMPORTANT findings. The one fresh observation (stdin buffered before size cap) is SUGGESTION-level — DoS-class, not credential-exfiltration, and gated on the attacker controlling both stdin source and the --body-file - flag. Prior blockers are all closed. Ready to merge.
Unblock summary (tower-dispatched worker)Result: merged via squash ( Review threads (4, all resolved)
New findingsThreads 3 & 4 surfaced after the pre-existing "ready to merge" snapshot and were not rubber-stamped: independently verified ( RefusedNothing refused — no finding fell in the never-defer classes (data-loss / fail-open safety-or-merge-gate / security / silently-wrong-into-irreversible-action). Signed-commit-only constraint honored (update-branch API, no contents-PUT). This was generated by AI (tower-dispatched unblock worker). |
1 participant
Summary
Standing operator principle: what needs no reasoning gets a script. Every
/looplane currently hand-assembles two pure-mechanics artifacts every session — the
MACHINE-BEHAVIOR telemetry block and the "maintain ONE telemetry comment, edit in
place" upsert. This PR extracts both into scripts the lane prompt references, so
the outputs are deterministic and testable and the lane prompt shrinks (context
minimalism — long prompts micromanage; Fable-class models need fewer steers).
Both scripts live under
plugins/claude-ops/skills/lanes/scripts/and follow theexisting
lane-launcher.shconventions (jq CRLF wrapper for native-Windows jq,the
--helpheader as the full contract, explicit exit codes, per-field gracefuldegradation for telemetry).
Fix
Part 1 —
machine-behavior.sh(issue item 1). Emits the MACHINE-BEHAVIORblock as a verbatim-printable text block: gh identity (
gh api user), clone pathgit worktree list --porcelain), and installed plugin versions.It deliberately does not compute "deviations from standing rules" — the
standing rules are prose the script cannot read, so a deviation is a model
judgment made by reading these facts against those rules. Emitting a fabricated
deviations verdict would put guessing inside the "deterministic" script; the
block ends with a one-line pointer reminding the lane to make that assessment.
For the same reason there is no heuristic "worktree pattern" field — the
worktrees legitimately span different parents, so the raw path list is the
pattern.
lane is actually running in. Per the lanes skill's
context/refresh.md, arunning lane's installed plugin version can lag repo HEAD mid-session — the
installed record (
installed_plugins.json) is the honest number, the checkoutis not.
--plugin <id>(repeatable) scopes the block to the plugins a lane runs.Part 2 —
telemetry-upsert.sh(issue item 2; interim home of the #502 contract).--issue N --marker STR --body-file PATHmaintains exactly ONE marker-identifiedcomment on a tracking issue, editing it in place instead of posting a second.
writes a machine-detectable sentinel as the comment's first line —
<!-- claude-ops:lane-telemetry marker=STR -->.STRis constrained to[A-Za-z0-9:._-]+so it can never contain the>that would close the HTMLcomment early. The sentinel is invisible in the rendered issue and distinct per
marker, so N lanes can each own one comment on the same issue without
colliding.
carrying the exact sentinel; (2) fallback — on first migration off a
hand-authored comment, the most recent comment by the authenticated user
carrying the raw marker text (PATCHing it adopts it, so the next cycle takes the
primary path). The comment list is fetched with
gh api --paginateand slurped;without pagination an existing comment on page 2 of a busy tracking issue is
invisible and the script would POST a duplicate — the exact bug it exists to
prevent. REST numeric comment ids are used for the
PATCH(the GraphQLgh issue view --json commentsnode ids the read-side uses are not accepted bythe REST
issues/comments/{id}endpoint).Part 3 — wire
re-anchor:script-the-deterministic-workinto the prompt-authoringritual (issue item 3): DEFERRED — blocked on #480. The issue scopes this to
"#480's future skill." #480 (a loop-prompt-authoring skill) does not exist yet
as an implemented skill — it is an open, unimplemented issue — so there is no
ritual to hook into, and building #480's skill is out of scope for this PR. A repo
grep for an existing prompt-versioning ritual/checklist (
prompt-version,prompt-authoring,script-the-deterministic) found only there-anchorskillitself and the existing
#480prompt-storage forward-dependency note in thelanes SKILL.md /
context/config.md— which is about storage, not the authoringritual, so wiring the re-anchor step there would be a stretch. Per the task's
stated scope boundary, no new ritual/checklist file is invented; Part 3 lands when
#480 does.
Verification
Tests follow this repo's convention (a sibling
.test.shper script, PATH-stubbedexternal commands + JSON fixtures, hermetic — no network/real CLI), discovered by
scripts/run-plugin-tests.sh(plugins/**/*.test.sh).machine-behavior.test.shcovers: gh identity rendering + degradation tounavailablewhen gh is missing/unauthenticated; worktree count, per-worktreebranch, detached-HEAD flag, and clone-path from the common git dir; installed
plugin versions filtered by
--plugin, with the scope-divergent marker when aname has >1 installed version; missing/malformed
installed_plugins.jsondegradation; a
--pluginnaming no installed plugin reported (not silently empty);argument/repo exit codes.
telemetry-upsert.test.shcovers: create when no match (POST); update-in-place onthe sentinel (PATCH to that id); fallback adoption of the user's raw-marker comment;
fallback refusing another user's marker comment (creates instead); pagination —
a sentinel comment on a second page is found (no duplicate POST); marker
>-injection rejection; non-numeric issue + missing body-file rejection;--dry-runperforms no write;
-reads the body from stdin.Repo quality gates run locally (all clean):
Closes #538
Related
double-post" rule
telemetry-upsert.shis the interim scripted home of.first (see the Fix section). Also the forward owner of durable prompt storage
referenced by the lanes skill today.
shrinks the per-cycle lane prompt.
subset of that work.
🤖 Generated with Claude Code
https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o