Repository navigation
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer with write access must add the See E2E testing guide for details. |
|
🤖 Finished Review · ✅ Success · Started 7:38 PM UTC · Completed 8:06 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $17.76 |
Site previewPreview: https://dbea793b-site.fullsend-ai.workers.dev Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
dhshah13
added
the
ok-to-test
github-actions
Bot
removed
the
ok-to-test
|
Risk Assessment: moderate (2/5) DetailsLarge change size and elevated git churn across touched runtime files are counterbalanced by an established contributor, zero protected or dependency modifications, a solid 0.31 test ratio, and close alignment with tracked feature issue #6970, preserving the moderate (2) risk rating anchored to the prior review. Previous runRisk Assessment: moderate (2/5) DetailsLarge change size and high git churn across touched runtime files are balanced by an established author, zero protected or dependency file modifications, strong test ratio, and close alignment with the tracked feature issue; unchanged from the prior review (score 2) since Tier 1 signals (author, protected/dependency-path counts, issue alignment) are the same. Previous run (2)Risk Assessment: moderate (2/5) DetailsLarge change size and high git churn across runtime files are balanced by an established author, zero protected or dependency file modifications, solid test coverage, and close alignment with the tracked feature issue. |
ReviewFindingsLow
All four high/medium findings from the prior review of an earlier commit in this PR were not observed in the current diff: Next steps:
Previous runReviewFindingsMedium
Low
All four high/medium findings from the prior review were verified against the current head and are not observed in the current diff: the sandbox-findings fail-open gap ( Next steps:
Previous run (2)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (3)ReviewFindingsHigh
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 8:22 PM UTC · Completed 8:52 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $12.23 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Review · Commit: |
|
Additional file-level and summary findings addressed in The output walker now exempts only the runner's exact audit locations. Agent-created Updated the flag documentation to state that clearing retains the existing write semantics but model fallback depends on the runtime. It now links both Pi and Codex, including Codex's environment/default fallback and its treatment of Claude frontmatter. Updated the developer tracing lists to match the emitted schema: when dollar cost is unavailable, This dependency is no longer unmerged: PR 7726 merged as All eight selected local native/Fullsend workflow audits passed for the source revisions documented in the PR. Final-head CI, authorized admin e2e and human approval remain separate gates. |
|
🤖 Review · ❌ Terminated · Started 9:46 PM UTC · Ended 10:23 PM UTC Commit: |
PR Summary by QodoSupport native Codex children with protected policy and complete evidence
AI Description
Diagram
High-Level Assessment
Files changed (78)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| Codex-specific model precedence and protected native role dispatch are defined | ||
| in [ADR 0126](0126-runner-owned-codex-child-policy.md). |
There was a problem hiding this comment.
5. An accepted decision edit lacks a callout 📘 Rule violation § Compliance
The PR adds a Codex-policy cross-reference to the already Accepted ADR 0104, but the PR description does not identify ADR 0104 or summarize that edit. The description discusses the new ADR 0126, so a reviewer of the accepted record is not explicitly told that the older record also changed.
Agent Prompt
## Issue description
The PR description does not identify the edit to accepted ADR 0104.
## Fix Focus Areas
- docs/ADRs/0104-per-persona-model-resolution.md[20-21]
## Recommended Fix
Add a PR-description note naming ADR 0104 and explaining that its Codex-specific policy now cross-references ADR 0126.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
The PR description now calls out the ADR 0104 cross-reference.
| log_finding("tirith_action_block", "critical", reason, "block") | ||
| return True, reason, "critical" |
There was a problem hiding this comment.
7. Below-threshold tirith blocks escalate to critical 🐞 Bug ≡ Correctness
When no individual finding meets TIRITH_FAIL_ON but Tirith's top-level action is block, check_command now logs and returns critical (previously high). The Codex adapter trusts this structured severity, so findings the operator deliberately set below the blocking threshold trigger the default critical halt.
Agent Prompt
## Issue description
The `action == "block"` fallback in `check_command` reports `critical` even though no finding met the threshold.
## Fix Focus Areas
- internal/security/hooks/tirith_check.py[233-240]
## Recommended Fix
- On the `action == "block"` path, return the highest severity among the findings Tirith reported, or `high` as before.
- Keep `critical` for malformed or unrecognized output and scanner failures.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
tirith_check.py is unchanged from main in this PR. The severity change, with this fix, moved to the security follow-up.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 9:46 PM UTC · Completed 10:22 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $21.42 |
Review and retro can now delegate to Codex native children (spawn_agent / wait_agent / close_agent, collaboration V1) instead of running every sub-agent in the root context. - Bootstrap registers skill personas plus default and explore roles as Codex role files, only for agents whose tools allow Agent (as on Claude and pi). Child models come from the runner: a persona mapping, FULLSEND_CODEX_SUBAGENT_MODEL, subagents.default, then gpt-5.6-luna. - A mandatory PreToolUse dispatch guard enforces fresh context, depth one, no call-site model or effort override, no resume and no V2 dispatch. Codex caps open children at four. - Codex starts under an inherited Landlock write restriction so the agent cannot rewrite role files, config, hooks or the auth helper that Codex re-reads when each child starts. Policy hooks run on a system Python outside the agent's write roots. - Child rollouts are collected and redacted as separate transcripts. Parent and child usage is reconciled per thread, and a run whose children did not complete and close, or whose usage or transcripts are missing, fails with ErrIncompleteEvidence instead of retrying. - Codex reports dollar cost as unavailable instead of zero. BREAKING CHANGE: Codex runs require a system python3 3.11+ outside /sandbox and /tmp and Linux Landlock ABI 3+ (kernel 6.2+) in the sandbox, including runs without children. During a Codex run, new top-level $HOME entries other than the ones Bootstrap pre-creates (.cache, .cargo, .config/git, .local, .npm) cannot be created. Skill sub-agent personas need a non-empty description. Codex child models must be OpenAI model IDs available in the pinned CLI; Claude aliases, inherit and other providers are rejected. Consumers of run metrics must treat cost_unavailable instead of reading a zero cost. Signed-off-by: Dharit Shah <dhshah@redhat.com>
Add ADR 0126 (runner-owned policy for native Codex children) and cross-reference it from the Accepted ADR 0104. Describe the Codex child roster, model precedence, dispatch limits, Landlock and Python requirements, cost_unavailable and the validated profile (Codex CLI 0.157.0 and 0.158.0, gpt-5.6-luna, medium effort) in the runtime, CLI, config and tracing docs. Signed-off-by: Dharit Shah <dhshah@redhat.com>
8e78100 to
4b6c863
Compare
|
🤖 Finished Review · ✅ Success · Started 2:22 AM UTC · Completed 2:48 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $14.28 |
| continue | ||
| } | ||
| if _, exists := children[id]; exists { | ||
| switch state.Status { |
There was a problem hiding this comment.
[low] logic-error
The agents_states switch at codex_run.go:514 has no default case, so an unlisted status leaves the prior children[id] value unchanged. Known agent-thread values handled are completed/pending_init/running/interrupted/errored/not_found. The parent prompt (codexSubagentNote, codex_personas.go:208) also names "shutdown" as a finished-child status alongside completed/errored/not_found, and that token is absent from the switch. A child that never reports completed stays false from spawn and still fails the run (fail-closed), so most unknown-status paths are safe. The residual gap: spawn + wait(completed, nonempty message) + a later unlisted status (e.g. shutdown) does not retract completeness, so close_agent can still record closure for a child whose actual terminal state was never recognized.
Suggested fix: Add a default case that sets children[id]=false for unknown agents_states.status values, or explicitly handle "shutdown" alongside the other incomplete terminals. Add a regression test covering spawn + completed nonempty wait + a later shutdown (or other unlisted) status.
| // flat_tool_name): V1 resume is multi_agent_v1resume_agent and V2 spawn is | ||
| // collaborationspawn_agent. The plain names cover providers without | ||
| // namespaced tools. | ||
| const codexDispatchMatcher = "spawn_agent|resume_agent|multi_agent_v1resume_agent|collaborationspawn_agent" |
There was a problem hiding this comment.
[low] fail-open
codexDispatchMatcher is a closed-world exact alternation: spawn_agent|resume_agent|multi_agent_v1resume_agent|collaborationspawn_agent. The comment documents Codex's flat_tool_name convention (namespace+name, no separator): V1 resume is multi_agent_v1resume_agent, V2 spawn is collaborationspawn_agent — but the equivalent V2 resume form, collaborationresume_agent, is absent from the matcher. Tests pin the observed names on CLI 0.157.0/0.158.0 and do not include this form, so it is unobserved on the pinned CLIs, not a demonstrated live gap. If a V2 parent ever emitted that concatenated resume name, Codex's PreToolUse hook lookup would not match it and the mandatory dispatch guard would never run for it (fail-open at the hook-dispatch layer), though native max_depth=1 and other guards remain as residual protection.
Suggested fix: Add collaborationresume_agent to codexDispatchMatcher if the pinned CLI can emit it, or document explicitly that V2 has no resume tool on 0.157/0.158. Extend the dispatch-policy test's observed-name list accordingly.
|
Hello! Do not submit ADRs and implementation at the same time, we still need to accept the ADR and if we reject it the way it is you just have wasted a lot of tokens for the implementation. Make this a draft and open a new PR with the ADR alone, where we can debate if it is worth an ADR or if we accept it/reject it. |
|
|
||
| Fullsend does not calculate inference cost from token counts or maintain a | ||
| model-price table. Each runtime reports a USD cost value and fullsend | ||
| model-price table. When a runtime reports a USD cost value, fullsend |
There was a problem hiding this comment.
Not needed for #6970: Codex has never reported dollar cost (ADR 0099), and this replaced the recorded $0 with an unavailable flag. Dropping it.
|
I feel this is handling too many things at once, any way you can split this? I bet there is work here that can be deferred to a followup PR. |
|
Closing in favour of small PRs after ADR 0126 (#7827) is decided; the split is in the comment above. |
Summary
Fleet
reviewandretrocan now run on the Codex runtime with native child agents (spawn_agent/wait_agent/close_agent, collaboration V1) instead of doing every sub-agent's work in one context. Paired with the instruction changes in fullsend-ai/agents#1542.Related Issue
Addresses #6970. Companion: fullsend-ai/agents#1542 (for fullsend-ai/agents#1500).
Changes
defaultandexplore, only for agents whose tools allowAgent(same rule as Claude Code and pi). Triage and prioritize keep the no-sub-agent path.FULLSEND_CODEX_SUBAGENT_MODEL→subagents.default→gpt-5.6-luna. Claude aliases,inheritand other providers are rejected.ErrIncompleteEvidence) without spending another retry.cost_unavailableinstead of$0.Moved out of this PR (they changed security behavior for every runtime and caused fleet regressions): the Codex security collector, the
on_critical: haltgate and the sharedtirith_check.pyseverity change. They are rebuilt with fixes on a follow-up branch for a separate issue. The Codex canary quoting bug onmainhas its own PR.BREAKING CHANGE:
$HOMEentries other than.cache,.cargo,.config/git,.localand.npmcan't be created.description.cost_unavailable.Testing
make lintpassesUnit tests pass for runtime, CLI, security and harness. The Landlock and dispatch-guard tests pass in both Linux sandbox images. About 99% of added Go lines are covered locally.
Live, local,
gpt-5.6-lunaat medium effort, Codex CLI 0.157.0 (pinned) and 0.158.0 (latest, #7747):npm install, tests passgo testpassesReview/retro audits check the selected roster, fresh context, at most 4 open children, every child completed and closed before output, one transcript per thread, reconciled usage and schema validation.
Security hooks cost turns but never stopped a run. Every code/fix run lost one command to
CANARY_LEAKED, caused by the canary quoting bug onmain; with that fix applied the same code run had none. Tirithanalysis_incompletealso blocked 1–3 nested shell commands per code/fix run, which is existing hook behavior.Review/retro use synthetic forge fixtures. Triage/code/fix ran against a disposable private repo with post-scripts off. Not covered: live forge posting, CI Workload Identity, GitLab runner kernels, V2 parents and mixed-model rosters.
Checklist
!for breaking changes)