STO-402: define deterministic Squad runtime contract#23
Conversation
📝 WalkthroughWalkthroughThe PR formalizes single-Squad ownership and issue contracts, adds deterministic staged execution and parent-result handling, rejects unsynchronized fallback leaders, updates Auto-Harness barriers, synchronizes supporting files, and adds documentation plus regression tests. ChangesSquad runtime contract
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant OwningSquad
participant ChildIssues
participant Orchestrator
participant ParentIssue
Caller->>OwningSquad: assign issue by UUID
OwningSquad->>ChildIssues: create validated staged work
ChildIssues->>Orchestrator: emit eligible barrier wake
Orchestrator->>ParentIssue: post or recover consolidated result
Orchestrator->>ParentIssue: verify metadata and update status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agents/orchestrator/skill.md`:
- Around line 183-186: Update the cancellation procedure to traverse the
complete descendant issue graph, replacing the subjective “relevant children”
scope with every child and descendant. Enumerate active tasks for each
discovered issue, cancel every active task, and verify none remain queued,
dispatched, running, waiting, or deferred across the full graph before setting
any issue status to cancelled.
- Around line 153-158: Update the parent close sequence in steps 1–4 to be
idempotent when command responses are lost: before retrying comment or metadata
writes, look up an existing authoritative result comment using the
correlation_id and parent comment thread. If found, reuse its comment UUID and
continue with metadata verification/activity/status steps without posting
another result; only create a new comment when no matching result exists.
In `@templates/auto-harness.md`:
- Line 193: Update the shell-command fenced code block in auto-harness.md to
specify the bash language identifier, changing the opening fence to bash while
preserving the block’s commands and closing fence.
In `@templates/squad-issue.md`:
- Around line 63-65: Use one deterministic fallback rule across all contract
surfaces: require a separate Orchestrator identity deployed in every affected
Squad and validated by a fresh topology verify. Update templates/squad-issue.md
lines 63-65 and workspace-context.md line 105 to include this requirement;
retain the stronger wording in AGENTS.md line 67, apply identical wording in
CLAUDE.md line 67, and verify AGENTS.md and CLAUDE.md are byte-identical with
cmp -s.
In `@tests/squad-runtime-contract.test.py`:
- Around line 79-98: Add assertions to
test_monitoring_recovery_and_cancellation_contract that verify orchestrator
contains the documented requirements to record both caller-configured and
runtime Multica CLI versions and to depend only on fields verified on both
sides. Use the repository’s exact documented wording and preserve the existing
command and recovery-order assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: becf2ef1-5adc-40c7-b1db-d87a0387651b
📒 Files selected for processing (12)
AGENTS.mdCLAUDE.mdREADME.en.mdREADME.mdagents/orchestrator/skill.mddocs/operating-model.mdscripts/sync-topology.pytemplates/auto-harness.mdtemplates/squad-issue.mdtests/squad-runtime-contract.test.pytests/sync-topology.test.pyworkspace-context.md
![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: 2b23a5d5fe
ℹ️ 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".
| ## Dispatch & DoD | ||
| ## Squad Entry, Dispatch & DoD | ||
|
|
||
| Assign a complex or multi-role goal to one exact owning Squad by UUID; do not have an external caller fan it out to several agents. A direct-agent path is allowed only when the work is trivial, single-owner, low-risk, has no cross-profession dependency, and needs no independent gate. Use `templates/squad-issue.md` for the Issue body. |
There was a problem hiding this comment.
For the existing multi-role PR review flow, .github/scripts/pr-sweep.sh:923 and :978 still reassign one issue directly among Engineer, Evaluator, and Orchestrator identities, while agents/orchestrator/skill.md:244 directly dispatches rework to the author. This new higher-precedence rule forbids that path because the flow has multiple professions and an independent gate, so deployed agents receive incompatible instructions on every non-approve review outcome. Either migrate PR-sweep issues to an owning Squad or define an explicit compatibility exception.
AGENTS.md reference: AGENTS.md:L61-L65
Useful? React with 👍 / 👎.
| tests/sync-topology.test.py | ||
| tests/sync-multica.test.sh | ||
| tests/pr-sweep.test.sh | ||
| tests/squad-runtime-contract.test.py |
There was a problem hiding this comment.
In a fresh checkout, this newly documented command exits with Permission denied because tests/squad-runtime-contract.test.py was added with mode 100644, despite its shebang and direct invocation here. Consequently, users and test runners following the quick-start commands cannot execute the new regression suite unless they know to invoke python3 manually; set the executable bit or document the interpreter explicitly.
Useful? React with 👍 / 👎.
|
Verdict: request-changes Reviewed immutable head Findings
Set the executable bit or document Reviewed-file coverageRead the complete diff and surrounding context for all 12 changed files: PR #22 remains open/ready at Verification re-run locallyThe focused suites passing does not clear findings 1–5: Read-only sync reproduction matched the PR body: No apply, branch edit, push, merge, deployment, or live workspace mutation was performed. PR template, rollback, and scope assessmentThe canonical PR body is ready-for-review and structurally complete: Its rollback section states Accepted choice: Request changes on this SHA before either independent gate can approve it. Rejected alternatives:
Constraint: STO-402 requires one internally consistent, deterministic Squad runtime contract, while this head still contains active paths that bypass Squad ownership, duplicate or omit final results, and leave cancellation scope ambiguous. Re-run all verification after these findings are fixed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fd67e81a0
ℹ️ 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".
| 5. A member returns a mention-free delivery addressing every evidence item. The current Squad leader verifies it and chooses the next transition. | ||
| 6. Use one Evaluator for ordinary independent verification. Use both Evaluators only for high-risk, irreversible, security-sensitive, or explicitly dual-lens work. They receive independent pre-verdict contexts and do not coordinate until both verdicts exist. | ||
| 7. Close only when the acceptance criteria are evidenced. Promote durable results to the Project ledger and record unresolved risks explicitly. | ||
| 7. Close only when the acceptance criteria are evidenced. Post one consolidated parent result, index it with the required result metadata, record Squad activity, and only then change parent status. Promote durable results to the Project ledger and record unresolved risks explicitly. |
There was a problem hiding this comment.
When both review lanes approve, .github/scripts/pr-sweep.sh:987-998 marks the review issue done directly, without a consolidated result comment, the five metadata keys, or Squad activity. Fresh evidence in this target is that the added PR-sweep compatibility paragraph exempts only its direct assignments, so the approve path still violates the newly mandatory close sequence on every successful sweep; either migrate that close path or explicitly define its alternative result contract.
AGENTS.md reference: AGENTS.md:L118-L120
Useful? React with 👍 / 👎.
| Assign complex or multi-role work to one exact owning Squad by UUID, not to several individual agents. Squad membership is a capability roster, not an instruction to activate everybody. For every run, the Orchestrator selects the smallest sufficient set of roles and states the distinct output expected from each activated member. | ||
|
|
||
| An Issue has exactly one owning Squad at a time. Other Squads may be consulted through a child Issue or explicit mention. Transfer ownership only through an artifact-backed handoff with acceptance criteria; do not make `Discovery -> Experience -> Delivery -> Growth -> Reliability` an automatic waterfall. | ||
| An Issue has exactly one owning Squad at a time, and its body uses `templates/squad-issue.md` as the execution and result contract. Other Squads may be consulted through a bounded child Issue or an artifact-backed mention that states the exact question and return artifact. Transfer ownership only through an artifact-backed handoff with acceptance criteria; do not make `Discovery -> Experience -> Delivery -> Growth -> Reliability` an automatic waterfall. |
There was a problem hiding this comment.
When PM wraps a discussion issue, agents/pm/skill.md:154-188 still instructs it to create each child unassigned and with only Outcome, Source, and Acceptance Criteria. Those children therefore have neither an exact owning Squad nor the required Squad Issue body fields such as correlation and result contract, contradicting this new unconditional Issue invariant; update that workflow or narrowly exempt these children.
AGENTS.md reference: AGENTS.md:L61-L65
Useful? React with 👍 / 👎.
|
|
||
| Required order: | ||
|
|
||
| 1. Post or recover one consolidated result comment on the parent issue. Derive `sha256:<correlation-hash>` from the caller-provided `correlation_id` and include exactly one inert marker as the final line of the result body: `<!-- squad-result: sha256:<correlation-hash> -->`. Never place the raw correlation value in this marker. Before the initial write and before every retry, search the trigger thread with `multica issue comment list <parent-issue-id> --thread <trigger-comment-id> --full --output json` for that exact marker. One match means the authoritative comment already exists: reuse that comment UUID and do not post. Zero matches means post once with `multica issue comment add <parent-issue-id> --parent <trigger-comment-id> --content-file <file> --output json` and capture its UUID. If the response is lost or ambiguous, perform the exact marker lookup before retrying. More than one exact marker match is an invariant violation: stop, preserve both IDs, and escalate without changing metadata or status. The comment states outcome, evidence/artifact links, deviations, rollout state, rollback, residual risks, and next owner. |
There was a problem hiding this comment.
When recovery is triggered by a different comment—for example, a caller retries with the same stable correlation ID after losing the first response—this lookup searches only the new trigger thread, so it cannot see the authoritative result posted under the original trigger and will post a duplicate. That breaks the one-result invariant and can later trip the multiple-marker escalation; deduplicate by the correlation-hash marker across all parent comments rather than only the current trigger thread.
AGENTS.md reference: AGENTS.md:L132-L136
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb3bc5dcba
ℹ️ 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".
| 1. Validate every proposed child carries all four suggested DoD fields: `outcome`, `evidence`, `verification`, `max_rounds`. Any entry missing a field, or a plan with zero entries, is malformed → rework dispatch back to the proposing Engineer naming the missing fields; this counts against that step's `max_rounds`. Do not create any children from a malformed plan. | ||
| 2. For a valid plan, create ONE child issue per entry (`multica issue create` with the entry's title and self-contained body, parented to the triggering issue), assigned to an Engineer instance — instance-neutral; either takes fresh work. Label every child at creation — `harness:cp` for each checkpoint-plan child, `harness:e2e` for the e2e-plan child — creating the label in the workspace first if it does not exist yet. Step 6 selects checkpoint children BY the `harness:cp` label, so an unlabeled child is invisible to the E2E hand-off and stalls the parent permanently. | ||
| 1. For `[auto-harness: checkpoint-plan]`, extract the authoritative fenced JSON array exactly into `./checkpoint-plan.json` with the file-write tool, locate this skill's supporting file `scripts/validate-checkpoint-plan.py`, then run `python3 <skill-dir>/scripts/validate-checkpoint-plan.py ./checkpoint-plan.json` before creating any child. Never substitute a same-named script from the target repository. The validator rejects zero entries; missing/invalid IDs, titles, bodies, stages, dependency lists, or DoD fields; duplicate/unknown/self/same-or-later-stage dependencies; Stage 1 dependencies; later stages without dependencies; and stages that are not one greater than their latest dependency stage. Any rejection is malformed → create zero children and rework-dispatch the exact error to the proposing Engineer; this counts against that step's `max_rounds`. For `[auto-harness: e2e-plan]`, retain the single-child four-field DoD validation because its eligibility is established only after the checkpoint barrier closes. | ||
| 2. For a valid plan, create ONE child issue per entry (`multica issue create` with the entry's title and self-contained body, parented to the triggering issue), assigned to an Engineer instance — instance-neutral; either takes fresh work. Before writing each checkpoint description file, prepend the validated `Checkpoint ID: <id>`, `Stage: <N>`, and `Depends on: <comma-separated IDs | none>` header so dependency identities survive child creation. Label every child at creation — `harness:cp` for each checkpoint-plan child, `harness:e2e` for the e2e-plan child — creating the label in the workspace first if it does not exist yet. For checkpoints, use the validator output verbatim: `--stage <validated stage> --status <validated status>`; the validator emits exact `--stage 1 --status todo` arguments for Stage 1 and `--stage <N> --status backlog` for every later stage. An unlabeled child remains invalid because the label identifies its harness role; stage/status controls its runnable frontier. |
There was a problem hiding this comment.
For every nontrivial auto-harness checkpoint, this creates the child assigned directly to an Engineer even though the child subsequently requires an independent Evaluator gate. Such a child fails the direct-agent fast-path criteria and has no exact owning Squad or templates/squad-issue.md contract, so the newly deployed auto-harness workflow contradicts the repository's mandatory routing model; create these children under the parent’s owning Squad and let its leader activate the Engineer lane.
AGENTS.md reference: AGENTS.md:L63-L65
Useful? React with 👍 / 👎.
| CHECKPOINT_ID = re.compile(r"cp-[0-9]+") | ||
| REQUIRED_FIELDS = {"id", "title", "stage", "depends_on", "body", "suggested_dod"} | ||
| REQUIRED_DOD_FIELDS = {"outcome", "evidence", "verification", "max_rounds"} | ||
| VERIFICATION_LEVELS = {"self", "evaluator", "dual_evaluator", "human"} |
There was a problem hiding this comment.
When a checkpoint selects verification: dual_evaluator, this validator accepts it, but the Orchestrator state machine at agents/orchestrator/skill.md:71-79 defines dispatch and return transitions only for a single evaluator; it has no state that records both independent verdicts and waits to reconcile them. Consequently a high-risk checkpoint can either stall after delivery or follow the generic Evaluator PASS branch and be marked done after only the first verdict, defeating the required dual gate; add explicit two-verdict state handling or reject this value until that exists.
AGENTS.md reference: AGENTS.md:L118-L120
Useful? React with 👍 / 👎.
| local_paths+=("${local_file#${files_root}/}") | ||
| done < <(find "$files_root" -type f ! -path '*/__pycache__/*' ! -name '*.pyc' -print | LC_ALL=C sort) | ||
|
|
||
| if ! python3 - "$remote_json" "${local_paths[@]}" > "$WORK_DIR/unmanaged-skill-files-$role" << 'PY' |
There was a problem hiding this comment.
When --apply encounters an existing remote supporting file absent locally, this check runs only inside sync_skill_files, after the main loop has already called sync_workspace_context and potentially updated the skill content; the script then continues to update agent instructions and attachments before exiting with an error. I reproduced this with the test fixture's unmanaged-file scenario and observed all four writes before the failure, so the claimed fail-closed desired-state check can leave a partially applied deployment; move this validation into the preflight phase before any remote write.
Useful? React with 👍 / 👎.
|
Verdict: request-changes Reviewed immutable head Findings
Seven-remediation regressionThe requested correction round is present and reproducible:
Reviewed-file and affected-caller coverageChanged files read completely: Affected unchanged callers/exports read: Verification re-run locallyFocused probes for uncovered behavior: No branch edit, push, merge, deployment, or live Multica mutation was performed. Generated PR body, rollback, scope, and PR #22Canonical body checks pass: The rollback plan states Accepted choice: Request changes on this immutable head before STO-402 advances. Rejected alternatives:
Constraint: STO-402 requires one internally consistent Squad runtime contract, but this head still has active paths that bypass exact Squad ownership, deterministic result closure, dual-gate reconciliation, or pre-write failure safety. Re-run the full suite and the six focused regressions after correction. |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
scripts/sync-multica.sh (2)
341-343: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuote the prefix inside
${var#...}(SC2295).
${files_root}is expanded as a glob pattern here. Role names are currently metachar-free so behavior is unaffected, but the same construct repeats at Lines 408 and 759 and is flagged by shellcheck.♻️ Proposed fix (apply at Lines 342, 408, 759)
- local_paths+=("${local_file#${files_root}/}") + local_paths+=("${local_file#"$files_root"/}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/sync-multica.sh` around lines 341 - 343, Update the local_paths prefix removal in the loops around local_file and the equivalent occurrences near the other reported locations to quote the files_root prefix within the parameter expansion, preventing it from being interpreted as a glob pattern while preserving the existing relative-path behavior.Source: Linters/SAST tools
345-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnmanaged-inventory check is duplicated verbatim in
preflight_skill_files.Lines 345-359 and Lines 762-774 run the same embedded Python and the same failure semantics, differing only in the log prefix. Extract one helper so the fail-closed rule cannot drift between the preflight and the apply phase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/sync-multica.sh` around lines 345 - 359, Extract the duplicated embedded-Python unmanaged-file validation and its shared failure behavior from the preflight and apply paths into one helper, then have both callers invoke it with their role, remote inventory, and managed paths. Preserve the existing fail-closed return status, diagnostic output, row recording, and error counting while allowing each phase to supply its distinct log prefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agents/orchestrator/skill.md`:
- Line 154: Update the metadata verification instructions in the issue-closing
workflow to validate allowed squad_verdict values, require squad_next_owner to
be the exact Squad name or “none,” and confirm squad_result_comment_id matches
the recovered or newly posted authoritative comment UUID. Perform all required
metadata checks before recording activity or changing status, and block status
changes if any validation fails.
In `@agents/pm/skill.md`:
- Around line 207-208: Define dual_evaluator in the PM DoD grammar and specify
its completion semantics, including two independent verdicts and deterministic
disagreement resolution, before permitting it in the Verification options.
Ensure the workflow has an explicit closure outcome for every dual_evaluator
request.
- Around line 162-164: Add a required exact owning-Squad decision to the Change
Proposal template and require it before the child-creation flow in the relevant
instructions. Ensure each independently created Issue resolves exactly one
owning Squad UUID from that proposal before writing the issue body from
templates/squad-issue.md.
- Line 167: Update both fenced code blocks in the skill document to include
language identifiers: use sh for the command block and markdown for the body
template, including the additionally referenced fence.
In `@docs/operating-model.md`:
- Line 57: The PR-sweep exception description in docs/operating-model.md at
lines 57-57 must state that it still follows the complete deterministic close
contract: authoritative result, metadata, activity, and status. In
agents/orchestrator/skill.md at lines 229-231, update the script-owned issue
workflow so it verifies the result and metadata before marking the issue done.
- Line 25: Resolve the Orchestrator’s cross-profession dependency for the retro
contract. In docs/operating-model.md at line 25, either define an explicit
allowed shared-template boundary or require the contract to be inline; in
agents/orchestrator/skill.md at lines 225-227, remove the direct Engineer
harness-procedure reference by embedding or relocating the retro format within
the Orchestrator bundle.
In `@templates/auto-harness.md`:
- Line 261: Reconcile Step 4 with the “No auto-harness path changes the parent
status directly” rule: either move the in_review transition to the CEO flow or
explicitly limit the rule to post-in_review terminal transitions. Update the
related contract assertion in squad-runtime-contract.test.py so it also catches
the lowercase Step 4 wording, preserving the intended ownership of parent status
changes.
In `@tests/pr-sweep.test.sh`:
- Around line 1093-1102: Strengthen the parent-result close-path assertions in
tests/pr-sweep.test.sh at lines 1093-1102, 1112-1118, and 1684-1686: add shared
checks covering all five metadata values/types, squad_result_comment_id,
squad_next_owner, unchanged correlation_id, result-comment linkage, and recorded
activity. Ensure each path verifies result → metadata → activity → status
ordering; apply the complete indexing assertions to the recovered and
inconclusive compensating close-outs as well.
In `@tests/sync-multica.test.sh`:
- Around line 17-24: The assert_no_contract_writes helper must also reject
“skill files upsert” operations so missing-skill preflight checks detect all
forbidden writes. Add this assertion while preserving a separate helper that
allows the intentional failed upsert scenario, and update the relevant preflight
test to use the stricter helper.
---
Nitpick comments:
In `@scripts/sync-multica.sh`:
- Around line 341-343: Update the local_paths prefix removal in the loops around
local_file and the equivalent occurrences near the other reported locations to
quote the files_root prefix within the parameter expansion, preventing it from
being interpreted as a glob pattern while preserving the existing relative-path
behavior.
- Around line 345-359: Extract the duplicated embedded-Python unmanaged-file
validation and its shared failure behavior from the preflight and apply paths
into one helper, then have both callers invoke it with their role, remote
inventory, and managed paths. Preserve the existing fail-closed return status,
diagnostic output, row recording, and error counting while allowing each phase
to supply its distinct log prefix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bc8a33e-8c60-47a0-a66e-6afd68f788dd
📒 Files selected for processing (18)
.github/scripts/pr-sweep.sh.github/workflows/pr-sweep.ymlAGENTS.mdCLAUDE.mdREADME.en.mdREADME.mdagents/orchestrator/files/scripts/validate-checkpoint-plan.pyagents/orchestrator/skill.mdagents/pm/skill.mddocs/operating-model.mdscripts/sync-multica.shtemplates/auto-harness.mdtemplates/squad-issue.mdtests/pr-sweep.test.shtests/readme-quickstart.test.pytests/squad-runtime-contract.test.pytests/sync-multica.test.shworkspace-context.md
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/squad-issue.md
| Required order: | ||
|
|
||
| 1. Post or recover one consolidated result comment on the parent issue. Derive `sha256:<correlation-hash>` from the caller-provided `correlation_id` and include exactly one inert marker as the final line of the result body: `<!-- squad-result: sha256:<correlation-hash> -->`. Never place the raw correlation value in this marker. Before the initial write and before every retry, search all comments on the parent with `multica issue comment list <parent-issue-id> --full --output json` for that exact marker; never scope recovery to the requested trigger thread. One match means the authoritative comment already exists—even when it belongs to a different top-level trigger after a duplicate caller retry: reuse that comment UUID and do not post. Zero matches means post once with `multica issue comment add <parent-issue-id> --parent <trigger-comment-id> --content-file <file> --output json` and capture its UUID. The requested trigger is only the parent for a genuinely new result, never the lookup boundary. If the response is lost or ambiguous, perform the same parent-wide marker lookup before retrying. More than one exact marker match is an invariant violation: stop, preserve both IDs, and escalate without changing metadata or status. The comment states outcome, evidence/artifact links, deviations, rollout state, rollback, residual risks, and next owner. | ||
| 2. Write and verify all five metadata keys with `multica issue metadata set`, using `--type string` for IDs/verdict/owner/correlation and `--type bool` for evidence completeness; then read them back with `multica issue metadata list <parent-id> --output json`. If any key is absent, mistyped, or the correlation value differs, do not change status. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate metadata semantics and result linkage before closing.
The verification step checks only presence, type, and correlation. It must also reject invalid squad_verdict values, ensure squad_next_owner is the exact Squad or none, and verify that squad_result_comment_id equals the recovered or newly posted authoritative comment UUID before activity/status changes.
Based on learnings, required result metadata must be verified before recording activity or changing status.
🧰 Tools
🪛 SkillSpector (2.3.11)
[warning] 340: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
[error] 153: [P2] Hidden Instructions: Hidden instructions were detected in comments or invisible text. These could contain malicious directives. Manual review is recommended.
Remediation: Audit all comments and invisible characters. Remove any instructions that direct the agent to perform unauthorized actions. Use plain, reviewable content.
(Prompt Injection (P2))
[error] 233: [P2] Hidden Instructions: Hidden instructions were detected in comments or invisible text. These could contain malicious directives. Manual review is recommended.
Remediation: Audit all comments and invisible characters. Remove any instructions that direct the agent to perform unauthorized actions. Use plain, reviewable content.
(Prompt Injection (P2))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agents/orchestrator/skill.md` at line 154, Update the metadata verification
instructions in the issue-closing workflow to validate allowed squad_verdict
values, require squad_next_owner to be the exact Squad name or “none,” and
confirm squad_result_comment_id matches the recovered or newly posted
authoritative comment UUID. Perform all required metadata checks before
recording activity or changing status, and block status changes if any
validation fails.
Source: Learnings
| For each genuinely independent item, first resolve the exact owning Squad UUID | ||
| from the accepted change proposal. Write its body from | ||
| `templates/squad-issue.md` (do not substitute the shorter discussion summary), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require ownership in the Change Proposal before splitting.
The Change Proposal template at Lines 130-152 has no owning-Squad field, so compliant proposals cannot deterministically provide the owner this flow requires. Add a required exact owning-Squad decision before child creation.
Based on learnings, each Issue must have exactly one owning Squad.
🧰 Tools
🪛 SkillSpector (2.3.11)
[info] 29: [EA3] Scope Creep: Skill's behavior or capabilities extend beyond its stated purpose. Scope creep allows an agent to perform actions unrelated to its documented functionality, increasing the attack surface.
Remediation: Limit the skill's scope to its documented purpose. Remove instructions that enable the agent to perform actions outside its stated functionality.
(Excessive Agency (EA3))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agents/pm/skill.md` around lines 162 - 164, Add a required exact owning-Squad
decision to the Change Proposal template and require it before the
child-creation flow in the relevant instructions. Ensure each independently
created Issue resolves exactly one owning Squad UUID from that proposal before
writing the issue body from templates/squad-issue.md.
Source: Learnings
| `templates/squad-issue.md` (do not substitute the shorter discussion summary), | ||
| then run: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to both fenced blocks.
These fences trigger markdownlint MD040; use sh for the command block and markdown for the body template.
Also applies to: 182-182
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 167-167: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 SkillSpector (2.3.11)
[info] 29: [EA3] Scope Creep: Skill's behavior or capabilities extend beyond its stated purpose. Scope creep allows an agent to perform actions unrelated to its documented functionality, increasing the attack surface.
Remediation: Limit the skill's scope to its documented purpose. Remove instructions that enable the agent to perform actions outside its stated functionality.
(Excessive Agency (EA3))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agents/pm/skill.md` at line 167, Update both fenced code blocks in the skill
document to include language identifiers: use sh for the command block and
markdown for the body template, including the additionally referenced fence.
Source: Linters/SAST tools
| ## Verification | ||
| <self | evaluator | dual_evaluator | human> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Define dual_evaluator completion semantics before allowing it.
This new value is absent from the PM DoD grammar and has no two-independent-verdict or disagreement-resolution path. A child can therefore request verification that the workflow cannot deterministically close.
🧰 Tools
🪛 SkillSpector (2.3.11)
[info] 29: [EA3] Scope Creep: Skill's behavior or capabilities extend beyond its stated purpose. Scope creep allows an agent to perform actions unrelated to its documented functionality, increasing the attack surface.
Remediation: Limit the skill's scope to its documented purpose. Remove instructions that enable the agent to perform actions outside its stated functionality.
(Excessive Agency (EA3))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agents/pm/skill.md` around lines 207 - 208, Define dual_evaluator in the PM
DoD grammar and specify its completion semantics, including two independent
verdicts and deterministic disagreement resolution, before permitting it in the
Verification options. Ensure the workflow has an explicit closure outcome for
every dual_evaluator request.
| | `.github/workflows/pr-sweep.yml` + `.github/scripts/pr-sweep.sh` + `tests/pr-sweep.test.sh` | Automated PR review chain. See "PR-Sweep Automation" | One Multica review issue per PR | | ||
|
|
||
| Each profession is two files. No shared skills, no cross-file references — every rule an agent needs is duplicated into that agent's `skill.md`. Duplication is the price of self-containment; we accept it. | ||
| Each profession owns its `personality.md`, `skill.md`, and any supporting files under `agents/<role>/files/`. No shared skills or cross-profession references: every rule and executable an agent needs belongs to that profession's skill bundle. Duplication is the price of self-containment; we accept it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Resolve the cross-profession skill dependency.
The repository now requires self-contained profession bundles, but the Orchestrator still depends on the Engineer's harness procedure for the retro contract.
docs/operating-model.md#L25-L25: define an allowed shared-template boundary or require the contract to be inline.agents/orchestrator/skill.md#L225-L227: remove the direct Engineer-procedure dependency by embedding or relocating the retro format.
📍 Affects 2 files
docs/operating-model.md#L25-L25(this comment)agents/orchestrator/skill.md#L225-L227
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/operating-model.md` at line 25, Resolve the Orchestrator’s
cross-profession dependency for the retro contract. In docs/operating-model.md
at line 25, either define an explicit allowed shared-template boundary or
require the contract to be inline; in agents/orchestrator/skill.md at lines
225-227, remove the direct Engineer harness-procedure reference by embedding or
relocating the retro format within the Orchestrator bundle.
Source: Coding guidelines
|
|
||
| Keep a direct-agent fast path only for work that is trivial, single-owner, low-risk, has no cross-profession dependency, and needs no independent gate. If any condition is false, the external caller assigns the owning Squad and lets its leader compose the lanes; it does not fan one goal out to several individual agents. | ||
|
|
||
| The checked-in PR-sweep is one narrow compatibility exception: it reuses one dedicated, serialized review issue and assigns that issue directly, one lane at a time, to the non-author Engineer, Evaluator, and Orchestrator. `.github/scripts/pr-sweep.sh` preserves the issue identity, ordered lanes, immutable head SHA, and Orchestrator-routed rework. This is not a reusable exception for other external callers or multi-role workflows. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Require the PR-sweep exception to preserve full closure semantics.
The exception is explicit about routing but not about completing the authoritative result → metadata → activity → status sequence.
docs/operating-model.md#L57-L57: state that the exception still requires the complete deterministic close contract.agents/orchestrator/skill.md#L229-L231: require the script-owned issue to verify the result and metadata before marking it done.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~57-~57: The official name of this software platform is spelled with a capital “H”.
Context: ... Engineer, Evaluator, and Orchestrator. .github/scripts/pr-sweep.sh preserves the issu...
(GITHUB)
📍 Affects 2 files
docs/operating-model.md#L57-L57(this comment)agents/orchestrator/skill.md#L229-L231
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/operating-model.md` at line 57, The PR-sweep exception description in
docs/operating-model.md at lines 57-57 must state that it still follows the
complete deterministic close contract: authoritative result, metadata, activity,
and status. In agents/orchestrator/skill.md at lines 229-231, update the
script-owned issue workflow so it verifies the result and metadata before
marking the issue done.
| per existing convention there. | ||
|
|
||
| Set parent status `in_review`. The CEO then runs its Retro close-out step: after the retro delivery passes the DoD check, the CEO closes the parent (`done`). Stop. | ||
| Keep the parent in `in_review`; do not change status here. The CEO then runs its Retro close-out step: after the retro delivery passes the DoD check, the CEO runs the Orchestrator skill's Deterministic Parent Close Sequence (correlation marker lookup/result comment → verified metadata → Squad activity → status). No auto-harness path changes the parent status directly. Stop. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
"No auto-harness path changes the parent status directly" contradicts Step 4.
Line 190 still instructs the assignee to "set parent status in_review" after posting the checkpoint plan, which is exactly an auto-harness path changing parent status. Reconcile the two: either scope this sentence to terminal transitions ("no auto-harness path closes or transitions the parent after in_review") or move the Step 4 transition to the CEO. Note the contract test only forbids the capitalized Set parent status \in_review` (tests/squad-runtime-contract.test.py` Line 151), so the lowercase Step 4 wording slips through — worth aligning the assertion too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@templates/auto-harness.md` at line 261, Reconcile Step 4 with the “No
auto-harness path changes the parent status directly” rule: either move the
in_review transition to the CEO flow or explicitly limit the rule to
post-in_review terminal transitions. Update the related contract assertion in
squad-runtime-contract.test.py so it also catches the lowercase Step 4 wording,
preserving the intended ownership of parent status changes.
| assert_comment_count "$tmp/captures" 1 | ||
| assert_update_count "$tmp/captures" 1 | ||
| assert_contains "$tmp/captures/issue-comment-1.md" "PR review accepted after two independent approvals." | ||
| assert_contains "$tmp/captures/issue-comment-1.md" "<!-- squad-result: sha256:" | ||
| assert_contains "$tmp/captures/metadata-squad_verdict.value" "delivered" | ||
| assert_contains "$tmp/captures/metadata-squad_evidence_complete.value" "true" | ||
| assert_contains "$tmp/captures/squad-activity.args" "squad activity 11111111-1111-1111-1111-000000000001 action" | ||
| assert_line_before "$tmp/captures/multica-calls.log" "issue comment add" "issue metadata set" | ||
| assert_line_before "$tmp/captures/multica-calls.log" "issue metadata list" "squad activity" | ||
| assert_line_before "$tmp/captures/multica-calls.log" "squad activity" "issue update 11111111-1111-1111-1111-000000000001 --status done" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete parent-result contract on every close path.
These tests cover only selected metadata fields, so regressions omitting squad_result_comment_id, squad_next_owner, or unchanged correlation_id can still mark the issue done. Add a shared assertion for all five verified fields, result-comment linkage, activity, and result → metadata → activity → status ordering.
tests/pr-sweep.test.sh#L1093-L1102: assert all five metadata values/types after the normal approve close-out.tests/pr-sweep.test.sh#L1112-L1118: assert recovered results are fully indexed, verified, recorded as activity, and only then closed.tests/pr-sweep.test.sh#L1684-L1686: assert the inconclusive compensating close-out also writes the complete result index.
📍 Affects 1 file
tests/pr-sweep.test.sh#L1093-L1102(this comment)tests/pr-sweep.test.sh#L1112-L1118tests/pr-sweep.test.sh#L1684-L1686
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/pr-sweep.test.sh` around lines 1093 - 1102, Strengthen the
parent-result close-path assertions in tests/pr-sweep.test.sh at lines
1093-1102, 1112-1118, and 1684-1686: add shared checks covering all five
metadata values/types, squad_result_comment_id, squad_next_owner, unchanged
correlation_id, result-comment linkage, and recorded activity. Ensure each path
verifies result → metadata → activity → status ordering; apply the complete
indexing assertions to the recovered and inconclusive compensating close-outs as
well.
| assert_no_contract_writes() { | ||
| local file="$1" | ||
| ! grep -Fq 'workspace update' "$file" || fail "failure path allowed a workspace write" | ||
| ! grep -Fq 'skill update' "$file" || fail "failure path allowed a skill-body write" | ||
| ! grep -Fq 'skill create' "$file" || fail "failure path allowed a skill-body create" | ||
| ! grep -Fq 'agent update' "$file" || fail "failure path allowed an agent write" | ||
| ! grep -Fq 'agent skills add' "$file" || fail "failure path allowed an attachment write" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include supporting-file upserts in the no-write preflight assertion.
assert_no_contract_writes omits skill files upsert, so the missing-skill preflight test at Line 304 would miss a forbidden supporting-file write. Use a stricter helper there; keep a separate helper for the intentional failed upsert scenario.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/sync-multica.test.sh` around lines 17 - 24, The
assert_no_contract_writes helper must also reject “skill files upsert”
operations so missing-skill preflight checks detect all forbidden writes. Add
this assertion while preserving a separate helper that allows the intentional
failed upsert scenario, and update the relevant preflight test to use the
stricter helper.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89be1bab6a
ℹ️ 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".
| if isinstance(data, dict): | ||
| data = data.get("comments") or data.get("items") or data.get("data") or [] | ||
| for item in data: | ||
| if marker in (item.get("content") or item.get("body") or ""): |
There was a problem hiding this comment.
When a PR contains prompt-injection text that causes either review lane to repeat the predictable marker—its hash input is fully derivable from the public owner, PR number, SHA, and verdict—this parent-wide substring search accepts that member comment as the authoritative result. One injected marker makes metadata point to a review delivery instead of the required consolidated result, while two markers permanently block closure; recovery must authenticate the automation-authored result rather than trust marker presence in arbitrary comments.
AGENTS.md reference: AGENTS.md:L132-L134
Useful? React with 👍 / 👎.
| elif [[ -z "$match" && -d "agents/$role/files" ]]; then | ||
| log "[preflight] $role skill '$skill_name' is missing but has required supporting files; cannot safely create and activate it before those files exist." | ||
| errors=$((errors + 1)) |
There was a problem hiding this comment.
On a fresh workspace where the mapped agents exist but Orchestrator Skill does not, --apply now always aborts here because the repository contains agents/orchestrator/files/. This makes the documented create-when-absent path—and the dry-run plan saying the file would be created after the skill—impossible to apply; create the still-unattached skill during the pre-activation phase, upload its supporting files, and only then update or attach the active contract.
Useful? React with 👍 / 👎.
| Preserve `spec` versus `impl` as a title/body classification until a caller | ||
| explicitly supplies the workspace's label UUID; the current CLI does not expose | ||
| a label-name flag on `issue create`. Do not invent or paste an unverified label | ||
| argument. |
There was a problem hiding this comment.
For an implementation child created by this wrap-up flow, recording impl only in its title or body leaves it without the label required by the PM trigger at lines 41 and 233. Consequently the generated child cannot later satisfy the documented “product review of an impl-label issue” transition; after creation, resolve the label UUID and use the separate label-add command, or change the downstream trigger to consume the preserved classification.
Useful? React with 👍 / 👎.
|
Verdict: request-changes Reviewed immutable head Finding
Smallest acceptable fix: do not claim the direct-agent PR-sweep exception implements the Squad-leader close sequence with this endpoint. Either (a) migrate the review Issue to an exact owning Squad and execute the activity step from a real leader task before terminal close, or (b) define a PR-sweep-specific audited activity/result primitive that is callable from CI and add an integration-shaped test enforcing the pinned endpoint's assignment/actor/task requirements. Do not merely skip the activity call: Because correction round 2 exhausted the automated author cap, this terminal non-approve verdict requires human escalation rather than a third author round. Thirteen-remediation regressionThe original seven remediations remain present and green: bounded serialized PR-sweep assignment, parent-result marker recovery, specialized deterministic close, recursive descendant cancellation, exact fallback eligibility, executable README quick starts, and producer → validator → consumer checkpoint graph preservation ( Five of the six terminal corrections are also valid:
The sixth correction—PR-sweep deterministic close—is structurally ordered but not executable against the pinned API for the finding above. Changed-file and affected-caller coverageRead completely: Affected context inspected: Engineer/Evaluator skills, deployment and five Squad manifests, Multica CLI Verification re-runIndependent zero-activation traces: Read-only live content checks reported workspace/skill drift and the validator missing; dry-run exited 0 and verify exited 1 with exactly three drift items. Exact-mapped topology dry-run exited 0 with three agent drift items; verify exited 1 with the same three. No apply or live mutation was performed. PR body, scope, rollback, and PR #22Canonical body validation passes: The rollback plan uses Accepted choice: Request changes and escalate this terminal finding to the human. Rejected alternatives:
Constraint: A terminal PR-sweep close must execute against Multica |
stone16
left a comment
stone16
left a comment
There was a problem hiding this comment.
Verdict: request changes.
Standards
- [P1] Runtime instructions depend on repository files that are not in the profession bundle.
agents/orchestrator/skill.md:214requires the deployed Orchestrator to build children fromtemplates/squad-issue.md, whiledocs/operating-model.md:25requires profession bundles to be self-contained and the Orchestrator supporting files contain only the validator. A valid checkpoint/E2E trigger therefore depends on a target-repository file the agent is not guaranteed to have. Inline the contract or ship it as an Orchestrator supporting file and test the deployed bundle. - [P1] The retro close-out has the same cross-profession hole.
agents/orchestrator/skill.md:227delegates the parent retro format to the Engineer's harness procedure. The Orchestrator cannot reliably construct/verify that DoD from its own bundle, so a completed E2E child can leave the parent permanently in review. Move the retro schema and acceptance semantics into the Orchestrator contract. - [P2] The accepted Change Proposal cannot supply the required owner.
agents/pm/skill.md:162-164requires the exact owning Squad to come from the accepted proposal, but the proposal template at:129-152has no owning-Squad field. Add and validate that decision before splitting.
Spec
- [P1] PR-sweep cannot execute its claimed activity→status close path. Review issues are created/reassigned to individual agents at
.github/scripts/pr-sweep.sh:632,:954, and:1009, then:1096callsmultica squad activity. Multica 0.4.11 rejects that endpoint unless the issue is assigned to a Squad and the authenticated actor is that Squad's leader with anX-Task-IDbelonging to the issue. GitHub Actions has neither leader-agent nor task context, so the approve path will stop beforedone/consensus. The stub attests/pr-sweep.test.sh:799-802always succeeds and masks this. - [P1] Result recovery trusts an unauthenticated, predictable marker.
.github/scripts/pr-sweep.sh:547-558accepts any comment containing a marker derived from public PR data. Prompt-injected review output can therefore be selected as the authoritative result (or two copies can permanently block closure). Authenticate the automation-authored result instead of matching arbitrary comment text. - [P2] Split implementation Issues lose the label required by their downstream trigger.
agents/pm/skill.md:175-178preservesimplonly in title/body, but the product-review route at:41and:231-233still requires animpllabel. Resolve the label UUID and callmultica issue label add, or change the downstream classifier.
All declared repository tests pass in an isolated worktree, but they do not exercise these production contracts.
1 participant
Originating Multica issue: STO-402
Original author: @hao
Summary
Makes complex work enter Multica through one exact owning Squad; defines one idempotent parent-result comment plus verified typed metadata, Squad activity, and status order; uses native staged checkpoint graphs; and makes supporting-file rollout fail before contract activation. The final correction also closes active PR-sweep, PM, auto-harness, dual-evaluator, duplicate-trigger, and partial-rollout bypasses.
Why
Closes STO-402 and implements the accepted corrections from STO-398. Rejected head
cb3bc5dcbaa1ee7c0c0971604e6fd62ba5657224retained active callers that closed without the result contract, created unowned or directly assigned multi-lane children, recovered correlation markers from only one trigger thread, accepted one verdict for a dual gate, and discovered supporting-file failures after contract writes. Any one of those paths violates deterministic Squad ownership or closure.Approach
The base STO-402 change keeps the mirrored company contract, workspace context, operating model, Squad Issue template, native stage validator, monitoring/recovery rules, recursive cancellation, honest fallback rule, and executable quick starts aligned. Correction round 2 changes only the affected callers/contracts/tests:
.github/scripts/pr-sweep.shnow recovers/posts one correlation-marked result, writes and verifies all five metadata keys, records Squad activity, and only then marks its review issue done..github/workflows/pr-sweep.ymlpins Multica CLI0.4.11plus the official release checksum because that is the first repository-tested caller version exposing metadata/activity.agents/pm/skill.mdassigns split children to one exact Squad UUID and copies the completetemplates/squad-issue.mdcontract.agents/orchestrator/skill.mdmakes result recovery parent-wide, records and reconciles two distinct Evaluator identities, and assigns auto-harness children to the parent's Squad.templates/auto-harness.mdconsumes exact Squad ownership for checkpoint and E2E children while retaining validatedstage/depends_onandtodo/backlogarguments.scripts/sync-multica.shvalidates all existing supporting-file inventories before the first write, upserts required files before workspace/skill-body/agent/attachment activation, stops immediately on file failure, and rejects creation of a missing skill that already requires files because the API cannot write its file before creating its body.Accepted choice: Preserve the existing architecture and close every active caller/failure-path gap in one author-produced head.
Rejected alternatives:
dual_evaluator; the current roster has two Evaluator identities and the accepted contract explicitly requires that gate for high-risk work.Constraint: One bypass or one partially activated contract is enough to make the runtime non-deterministic.
Final correction round 2 — finding resolution
Branch:
agent/hao/sto-402-squad-runtime-contractFinal immutable head:
89be1bab6a49a63ab6ef6d4148efcf398abad70eRejected head retained only for before-evidence:
cb3bc5dcbaa1ee7c0c0971604e6fd62ba5657224.github/scripts/pr-sweep.sh:1015-1111implements marker recovery/result → five verified metadata keys → Squad activity → status.tests/pr-sweep.test.sh:1088-1118asserts order and parent-wide result reuse;:1663-1692proves failures do not produce a terminal consensus or raw close.agents/pm/skill.md:162-229resolves an owning Squad UUID, uses--assignee-id, and copies every Squad Issue section.tests/squad-runtime-contract.test.py:77-84rejects the old empty-assignee/short-body path.agents/orchestrator/skill.md:153searches all parent comments before initial/retry writes and reuses a match from a different top-level trigger..github/scripts/pr-sweep.sh:539-558andtests/pr-sweep.test.sh:1108-1118execute the same duplicate-trigger invariant.agents/orchestrator/skill.md:213-227andtemplates/auto-harness.md:142-188,220-239assign checkpoint/E2E children to the parent's exact Squad usingtemplates/squad-issue.md; the leader then activates Engineer/Evaluator lanes.tests/squad-runtime-contract.test.py:197-202,249-257rejects direct Engineer assignment while retaining graph arguments.agents/orchestrator/skill.md:70-79records two expected Evaluator UUIDs, treats duplicate identity delivery as retry, waits for both verdicts, requires two PASS results, and routes either FAIL to the original executor.tests/squad-runtime-contract.test.py:260-268locks those transitions.scripts/sync-multica.sh:685-809validates all inventories before writes and runs supporting-file upserts before activation;:320-401stops the file phase on error.tests/sync-multica.test.sh:297-334proves missing-skill, unmanaged-file apply, and injected upsert failure paths have no workspace/skill-body/agent/attachment writes.All seven prior remediations remain: the bounded serialized PR-sweep exception, lost-response recovery, specialized deterministic close, recursive descendant cancellation, exact fallback eligibility, executable quick starts, and producer → validator → consumer checkpoint graph preservation.
Rollout plan — not performed
89be1bab6a49a63ab6ef6d4148efcf398abad70e.main == origin/main.--verify, then exercise one disposable Squad result and both dual-evaluator identities.Duplicate triggers use the same stable correlation ID and parent-wide marker lookup. One match reuses the original result comment UUID; more than one match stops closure for human repair. A changed PR head uses a new correlation and requires two new lane verdicts.
How I Tested
End-to-end and structural cases
tests/pr-sweep.test.sh:1088tests/pr-sweep.test.sh:1108--threadlookup.tests/squad-runtime-contract.test.py:77tests/squad-runtime-contract.test.py:160and:249tests/squad-runtime-contract.test.py:260tests/sync-multica.test.sh:307tests/sync-multica.test.sh:326tests/sync-multica.test.sh:297tests/squad-runtime-contract.test.py:87-306Failing-before output on rejected head
The final structural test file was copied into a detached
cb3bc5dworktree and run against that head:Ryan and Evaluator-A independently reproduced the old apply path exiting 1 only after
workspace update,skill update,agent update, andagent skills add;tests/sync-multica.test.sh:307-334is the now-passing executable regression for both unmanaged inventory and injected upsert failure.Passing output
Caller/runtime and read-only live evidence
Exact-mapped read-only topology plan/verify did not guess or write. Both reported the same drift before failing closed on an existing unmanaged extra Growth member (
Hao): Orchestratorruntime,model; Evaluator-Aruntime; Evaluator-Bruntime; Delivery Engineer roles differ; every reported Squad definition was up to date. Automatic member removal remains intentionally forbidden.PR #22 is still open/ready at
659d08f4f8f284cb66cb6cd7824f9c6f5d0bf8bcand was not modified. PR #23 is open/ready/mergeable at the final SHA. No--apply, live issue/agent/workspace/skill/topology mutation, merge, deployment, or rollout occurred.Rollback Plan
Revert with
git revert <merge-commit>. If a later separately authorized content rollout occurred, sync from the reverted cleanmainand run fresh--verify. Maximum blast radius is single feature (Squad orchestration/review automation contracts). Time-to-rollback is under one minute before rollout or one content-sync cycle after rollout. No schema/data/dependency migration exists.Out of Scope
Summary by CodeRabbit