refactor: objective-scoped orchestration, portable skill contract, Gemini CLI target (3.0.0) - #90
Open
Tamircohen28 wants to merge 9 commits into
Open
refactor: objective-scoped orchestration, portable skill contract, Gemini CLI target (3.0.0)#90Tamircohen28 wants to merge 9 commits into
Tamircohen28 wants to merge 9 commits into
Conversation
…ni target
Change the canonical abstraction from "one agent task = one PR" to
"one user objective = one PR", and make the methodology provider-neutral
with capability-aware platform adapters.
Workflow
- add orchestrate-dev (objective -> DAG -> workers -> integration -> ONE PR)
- add worker-dev (commit + Tier 1 validation + handoff; never opens a PR)
- add deliver-dev (combined review, Tier 2 gates, single PR, hands to pr-dev)
- keep /start-dev as a compatibility facade; no user-facing break
- plan-dev emits an objective + task DAG; GitHub issues become optional export
- pr-dev is now the delivery lifecycle driver; auto-merge is policy, not invariant
Core
- core/capabilities: 6 platforms x 19 capabilities, every row explicit
- core/roles, core/policies, core/workflow schemas (objective/task/handoff)
- validation tiers 0-3; every skill and script declares the tier it invokes
- plugin-version.json as the single version source + check-version-truth.sh
Skills
- retire "all 16 Claude fields on every SKILL.md"; portable core is name +
description, with metadata.tamirs and Claude fields as validated extensions
- --profile claude-strict preserves the old gate so nothing regresses
- role and capability vocabularies are read from core/ at runtime, not hardcoded
Platforms
- Gemini CLI as a first-class target: generated, drift-checked extension mirror
(its loader globs one level, so canonical skills/<domain>/<name> loads zero)
- OpenCode modernized to native skill discovery; agents remain generated
- platforms/<id>/adapter.yaml for all five, joined to the registry by registry_key
Fixes found by the new checks
- make lint covered 47 of 118 shell scripts; -maxdepth 1 hid hooks/lib and every
skills/**/scripts/*.sh, and swallowed failures inside a pipeline subshell
- scaffold.sh used bash 4+ ${var^^} and an unbound $PREFIX_; it could not run on
macOS bash 3.2 at all. Both language paths now verified end to end
- validate_artifact.sh had a shell syntax error and had never parsed
- statusline resolves from the plugin root and cannot block on absent stdin
- version drift: README badge and platform-targets reviewed_by_skill were 2.0.0
Evidence
- make validate exits 0; 361 assertions across 10 suites
- orchestration simulation (114 assertions, no model calls) proves no worker
opens a PR, dependencies are respected, state resumes, and the sequential
no-subagent path reaches the same final state as the parallel one
- Gemini and OpenCode capability rows are measured against real CLIs
(0.55.1 / 1.18.11), not inferred from documentation
Phase 0 inventory of all 451 tracked files: docs/engineering/refactor/
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MAJOR per docs/engineering/build-and-release/versioning.md, which names "a changed skill contract" as a major bump. The 16-field Claude frontmatter requirement is retired in favour of a portable core plus validated extensions. No user-facing break is intended: /start-dev remains a working facade, no skill was removed or renamed, and --profile claude-strict still enforces the old gate. Edited plugin-version.json only; the six consumers were rewritten by scripts/check-version-truth.sh --sync. Version truth now reports 0 warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…racts The Phase 0 audit found CI never ran `make validate`, so check-doc-claims, check-feature-equivalence, check-marketplace-schema and opencode-agents-check only ever ran on the maintainer's machine — which is how the README version badge and the stale skill count reached the audit baseline unnoticed. Adds full-validate, orchestration-tests, skill-contract, static-and-docs, and a platform-contract matrix (gemini, opencode) that runs each suite's always-on schema half and skips the live-CLI half on ubuntu-latest. Adds concurrency cancellation for superseded PR commits. ubuntu-latest only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
A row describes THIS REPO'S ARTIFACTS on that platform, not the platform's abstract feature set. The definition was ambiguous between the two readings and two people reading the same row reached different answers. The chosen reading is what every existing row already means: cursor.hooks is partial because our bundle does not fully run there, and opencode.hooks is unsupported because we ship no plugin module — not because OpenCode lacks a plugin API. The platform-abstract reading would make both of those rows wrong. It is also the more useful question: readers are deciding whether their hooks run after installing this plugin, not comparing vendors. gemini_cli.hooks stays `unknown`. An escalation to move it to `unsupported` rested on "Gemini's event vocabulary is disjoint from Claude's", which the shipped 0.55.1 bundle disproves: it carries an explicit PreToolUse -> BeforeTool migration table, and SessionStart / SessionEnd / Notification are shared outright. Three of this repo's ten declared events exist on Gemini and nobody has tested whether they fire, so `unsupported` would assert a measured failure that was never measured. Also extends Provenance: ask how a claim was measured before building on it, including when it arrives from a teammate. A secondhand assertion is a declaration wearing a measurement's clothes and does not become evidence by being repeated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bounding the hook stdin read with `read -t` (correct: timeout(1) is absent on macOS) turned a hang into an empty string, which made a previously unreachable path live. On an empty or unparseable payload the slug degraded to a placeholder and the hook created a real worktree plus a `wt/session` branch — one leaked worktree and branch per stray invocation. Empty, non-JSON, and slugless payloads are now a clean no-op; a well-formed payload still creates its worktree. Also hardens the detector that found it. The stdin scan matched `=$(cat)` but not `="$(cat)"` — the spelling 14 of this repo's 19 stdin-reading hooks actually used — so it under-reported by three quarters and reported clean, which on a healthy repo is indistinguishable from working. It now matches the quoted form, ignores comments describing the defect, and carries positive-control fixtures pinning both matching rules and both guard forms: a scanner that cannot fail proves nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the empty-payload worktree leak. The guard is now a single emit_noop_and_exit helper used by every "I have no usable information" exit, rather than a check at one entry point. - an empty payload, or one that is not valid JSON, exits before jq runs. The previous behaviour let jq fail mid-script, which exited non-zero and disrupted the harness for no benefit; the hook now always emits a minimal valid UserPromptSubmit response and exits 0. - a prompt that yields nothing sluggable (absent, or only whitespace and punctuation) takes the same path instead of falling through to a placeholder. Defaulting an empty slug is what turned "no information" into an invented worktree and a wt/session branch. Verified per the repo rule for touching hooks/lib/worktree-common.sh: shellcheck clean, and both capture-task-slug.sh and worktree-create.sh exercised — tests/test-hook-stdin.sh 54/0 and tests/test-worktree-objective.sh 39/0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ts dying at 127
Two additions from the empty-payload postmortem.
A static check for the shape behind both real bugs:
short_id="${session_id:0:8}" # empty when there is no session id
task_slug="session-${short_id}" # -> "session-", a real branch name
A default correct for one caller shape that INVENTS information for another —
`is_git_repo ""` falling through `${1:-.}` to `.` is the same mistake. Scope is
stated in the source rather than implied: identifier-sized substrings only
(N<=16, since `${url:0:120}` is display truncation and was 10 of 13 hits before
the bound), a guard anywhere in a file exonerates that file, and a hit is not
automatically a bug. It therefore under-reports and is advisory, not a gate — it
catches "never guarded at all", which is what both real bugs looked like.
The check ships with positive and negative controls, as the stdin detector now
does: a scanner that cannot fail proves nothing.
Separately, tests/contract/*.sh and tests/orchestration/scenario-*.sh are
fragments sourced by their runners. Run directly they died on the first helper
call with an opaque 127; they now say what to run instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…it armed
`session_slug="${session_title:-session-${short_id}}"` was unreachable — the
guard at line 103 exits on an empty or "null" session_title before this point.
But it is the exact shape that produced `wt/session-`, and a dead default is one
moved guard away from being live again. The invariant is now stated in a comment
and the branch uses $session_title directly.
Verified: guard confirmed at line 103; test-hook-stdin 54/0,
test-worktree-objective 39/0, make validate exit 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sub-agents share one worktree filesystem, so two agents editing the same file is silent last-write-wins loss rather than a git conflict. Partition by exclusive file ownership and route cross-boundary changes through a request mailbox the orchestrator applies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the full refactor specified in the architecture report: the canonical abstraction moves from "one agent task = one PR" to "one user objective = one PR", and the methodology becomes provider-neutral with capability-aware platform adapters.
Built by 13 parallel sub-agents on strictly partitioned file ownership, with cross-boundary changes routed through a request mailbox.
The core change
A worker now ends at commit + handoff. It is explicitly forbidden from opening a PR, enabling auto-merge, merging the base branch, or running the full repo suite.
deliver-devis the only place an objective's PR is created.The sequential, no-subagent path is first-class, not a footnote — same task graph, same handoffs, same integration, same single PR, only the concurrency is gone. A test asserts both paths reach the same final state.
What landed
orchestrate-dev,worker-dev,deliver-dev;/start-devkept as a compatibility facadecore/{capabilities,roles,policies,workflow,providers,schemas}— 6 platforms × 19 capabilities, every row explicitname+description, withmetadata.tamirsand Claude fields as validated extensionsplatforms/<id>/adapter.yamlfor all fiveplugin-version.jsonis the single source; six consumers synced by toolingDefects found and fixed
The new checks immediately caught real, pre-existing bugs:
make lintcovered 47 of 118 shell scripts. A single-maxdepth 1excluded all ofhooks/lib/— includingworktree-common.sh, whichCLAUDE.mdrequires be shellchecked — and everyskills/**/scripts/*.sh. It also swallowed failures inside a pipeline subshell, printing "shellcheck passed" over real findings.scaffold.shcould never run.${var^^}is bash 4+ and macOS ships 3.2; underneath sats/PREFIX_/$PREFIX_/g, where$PREFIX_is an undefined variable that aborts underset -u. Both language paths now verified end to end on stock macOS bash.validate_artifact.shhad a shell syntax error and had never parsed.skills/<domain>/<name>/tree resolves to zero, with only a debug warning. Fixed with a generated, drift-checked mirror whose tests assert an empty mirror fails.gemini_cliwas built from the spec's prose at 9native; measurement against CLI 0.55.1 found subagents rejected (tools.0: Invalid tool name) and statusline unsupported. Nine became five — before it propagated into the published support matrix via the derived mirror.platform-targets.jsonwere still on 2.0.0.Evidence
The orchestration harness (114 assertions, no model calls) proves the invariants: no worker opens a PR, dependencies are respected, failures don't corrupt objective state, objectives resume after restart, multi-PR delivery is refused without a stated exception, and the sequential path matches the parallel one. It also greps the shipped
worker-dev/SKILL.mdto confirm it says so, and that no shared script callsgh pr create.Gemini and OpenCode capability rows are measured against real CLIs (0.55.1 / 1.18.11), not inferred from docs.
Backward compatibility
/start-devstill works for simple tasks. No skill was removed or renamed.--profile claude-strictstill enforces the old full-field gate, so nothing that passed before regresses. Legacy platform-shaped worktrees stay understood and are never orphaned.3.0.0 is MAJOR per this repo's own policy, which names "a changed skill contract" as a major bump — no user-facing break is intended.
Also included
Phase 0 inventory of all 451 tracked files (
docs/engineering/refactor/), which found that 42% had no validator at all — the finding that drove most of the fixes above.Follow-ups deliberately not done here
skills/<domain>/<name>/toskills/<name>/(would make every skill resolve twice under Claude Code and OpenCode).--require-tamirs— correct only once every skill carries the block;--require-tamirsexiting 0 is the readiness test.