fix(docs): clarify inline model behavior without project config - #280
Conversation
Session-Id: 01a08ba4-4473-7903-b7c3-e778edb9b523 Session-Id: 01a08ba4-4473-7903-b7c3-e778edb9b523
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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 |
Review swarm: maintainabilityMaintainability Review: PR #280PR: #280 - docs: correct inline named-agent example after the #263 fix SummaryThis PR modifies Maintainability AssessmentFinding 1: Implicit contract between documentation and implementation behavior (CRITICAL)Location: Lines 166-180 in the modified section Issue: The documentation asserts complex behavioral contracts without any reference to:
The text describes four distinct policy paths:
Maintainability risk: Six months from now, someone reading this cannot determine:
Evidence from the codebase context: Line 197-201 acknowledges "this named-agent contract currently ships in the canonical declarative YAML/JSON compiler" and notes matching TypeScript types are unmerged in PR #134. This means:
Finding 2: Missing failure mode specificationLocation: Lines 169-173 Issue: The text describes what happens when
RFC-0001 covenant 2 requires "every failure is one of a closed set of declared kinds" with Maintainability risk: When the described behavior fails, what error appears? The absence of this mapping means:
Finding 3: The word "still" asserts a relationship the code cannot enforceLocation: Line 171 Text: "A model declared directly on a step still requires the project allowlist." Issue: The word "still" implies continuity with a prior rule, but the prior sentence says "inline named-agent declarations proceed to the real CLI/model probe without a registry." This creates an unresolvable logical dependency:
Maintainability risk: The sentence is ambiguous in precisely the case a maintainer would need clarity — when no config exists. The implementation must resolve this ambiguity, but the documentation does not state which resolution is correct. Finding 4: No named boundary between "project allowlist" and "registry"Location: Throughout the modified section (lines 166-180) Issue: The text uses both "registry" and "project allowlist" to refer to the same
Maintainability risk: Two terms for one concept creates grep ambiguity. A search for "registry" will not find the "project allowlist" requirement, and vice versa. The distinction between these terms (if any) is undefined. Finding 5: Test mutation claim cannot be verified from this changeLocation: The PR title references "#263 fix" Issue: The PR title asserts this corrects documentation "after the #263 fix," implying:
However, the diff contains no:
Maintainability risk: A reader encountering this in git history six months from now cannot:
Relevant standard: AGENTS.md lines 96-98: "Cite paths that exist. A transcript path in a report is checked; a wrong one reads as fabrication even when the work is real." Finding 6: Implementation status caveat creates two sources of truthLocation: Lines 197-201 Text: "Implementation status for issue #132: this named-agent contract currently ships in the canonical declarative YAML/JSON compiler. Matching Issue: This paragraph is metadata about implementation status embedded in a specification section. It will become stale when:
Maintainability risk:
Finding 7: Example shows the feature but not the failureLocation: Line 169-170 Text: "inline named-agent declarations ( Issue: The example shows valid syntax but does not show:
Maintainability context: This is a docs change responding to a fix (#263). The reader needs to know what breaks and how, not just what works. RFC-0001 covenant 1 requires "error messages name the author's mistake in the author's vocabulary." Bounded approval conditionsThis change can be maintainable IF:
Verdict rationaleThe documentation describes behavior that cannot be verified, debugged, or safely modified by someone who has not already read the implementation. The three-way policy fork (no config / empty list / populated list) is critical to covenant 2 (no unexpected failures) but is described without:
Per RFC-0001 §1: "A relayflow may fail only in ways it declared" and covenant 2's preflight requirement. This documentation adds policy branches without declaring their failure taxonomy. Per AGENTS.md line 98: "Cite paths that exist." The implementation location and test coverage are not cited. The change itself may be correct, but it is not maintainable in its current form because a stranger reading this in six months cannot change it safely. REVIEW_FAILED |
Review swarm: historyPR #280 — history reviewReviewed head: Verdict and reasoningNo blocking history findings. This documentation correction follows the behavior deliberately landed in its immediate parent, The original registry contract came from RFC-0001 covenant 1 favors self-contained authoring, while covenant 2 requires preflight rather than guessing model readiness. This documentation fits both: the no-config case reaches a real probe, not assumed success. It does not change the closed kernel vocabulary or move provider policy into the kernel (settled decisions 5 and 13), nor edit the reviewing gate (decision 6). None of the settled decisions requires a project registry for every inline named agent. Relevant DRIVE-LOG lessons were checked against the actual parent and patch: the #252 entry at lines 7553–7600 warns about behavioral claims unsupported by code; the stale-checkout correction at lines 9659–9698 warns against treating old behavior as current; the paired 10:43Z/10:57Z entries at lines 10248–10328 retract the claim that a missing /tmp diff meant the history reviewer never saw the supplied patch. Here the staged diff is compared byte-for-byte with the recovered commit diff, and the documented behavior is traced to its merged parent. The older nearest-config incident at lines 1305–1315 remains respected: absence anywhere in ancestry differs from an existing empty policy; there is no parent-config merge or implicit fallback introduced.
Input recovery and limitsThe initial Reading the requested temporary diff failed with: Used git clone --bare --single-branch --branch fix/docs-inline-model-shakedown-0910 https://github.com/AgentWorkforce/flows.git /home/daytona/.project-gitCaptured output (exit 0): Then attached that metadata to the existing snapshot and populated its index without checking out files: git config core.bare false
git config core.worktree /project/workflows/runs/e4bb3e02-d69d-4a2e-9a96-7ce488083d8c
git read-tree HEADThose commands produced no output. Existing snapshot file-mode differences are outside the PR and were left alone. This is a static history review, not a runtime or test-suite signoff. Captured evidenceEach block below contains the literal command, its output, and exit status. Empty output blocks mean the command printed nothing. git rev-parse HEADExit status: 0. git log --oneline -40Exit status: 0. git show -s --format=fuller HEADExit status: 0. git diff --name-status HEAD^ HEADExit status: 0. git diff --abbrev=8 HEAD^ HEAD -- docs/SURFACE.mdExit status: 0. git diff --abbrev=8 HEAD^ HEAD -- docs/SURFACE.md | cmp - .review-target/pr.diffExit status: 0. git diff --exit-code HEAD -- docs/SURFACE.md packages/sdk/src/preflight.ts packages/sdk/tests/preflight.test.tsExit status: 0. git show a42ca16 -- packages/sdk/src/preflight.ts packages/sdk/tests/preflight.test.tsExit status: 0. git show 990093b:docs/SURFACE.md | sed -n '161,175p'Exit status: 0. sed -n '179,225p' packages/sdk/src/preflight.tsExit status: 0. rg -n 'modelRegistryPath|models:' packages/sdk/src/cli/check.tsExit status: 0. REVIEW_PASSED |
Review swarm: structureNo fresh transcript was produced for run |
Review swarm: FAILED
Cloud run: |
kjgbot
changed the title
maintainability lens — PASSMaintainability review — PR #280 (docs/SURFACE.md)This is a documentation-only diff. The task is to specify the three-state model-registry behavior for Concerns
Notes
None of the above is load-bearing enough to block. The specification's contract is recoverable with a re-read; the code behavior is unchanged and gated by tests referenced in a42ca16. REVIEW_PASSED |
history lens — PASSBlockers: none.
Concerns: The neighboring, unchanged statement at Notes: I did not rerun the launch-shakedown commands reported in the PR body; those remain author-supplied evidence. The stale gate brief in REVIEW_PASSED |
structure lens — MISSING |
|
🎯 review-swarm: FAILED (M:pass H:pass S:missing) Lens transcripts posted as sibling comments above. |
2 participants
The surface docs still say all models require a project registry, contradicting #266. Document the actual split: inline named-agent models reach the live readiness probe when no
flows.jsonexists, while direct step models still need an allowlist; an existing config still enforces its policy.Validated against
preflight.ts:180and two actualflows runoutcomes from the launch shakedown (both outside any config ancestry):A forward-only PATH shim captured the real inline-model probe before the auth refusal:
git diff --checkexited 0 with no output. Documentation-only change.Note
Low Risk
Documentation-only change to SURFACE.md; no runtime or preflight code is modified.
Overview
Updates Deterministic model registry in
docs/SURFACE.mdso it matches post-#266 preflight behavior instead of implying every declared model must hit aflows.jsonallowlist before probing.When no
flows.jsonexists in the flow file’s ancestry, inline named-agent models (agents: { …: { cli, model } }) may proceed to the live CLI/model probe; step-levelmodeldeclarations still require an allowlist and can refuse withmodel_unknown. When a config is found, unlisted models are refused up front—including named agents—and an empty or missingmodelsfield is documented as an explicit deny policy. Wording around the pure first pass and allowlisted probes is tightened to reflect that split.Reviewed by Cursor Bugbot for commit e88651b. Bugbot is set up for automated code reviews on this repo. Configure here.