cleveragents/cleveragents-core
cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.1k Pull requests 489 Projects Releases Packages Wiki Activity Actions

docs: document v3.8.1 changes — skeleton context inheritance, actor YAML-first path, checkpoint triggers #3696

Closed
freemo wants to merge 1 commit from docs/ca-docs-writer-v3.8.1-2026-04-05 into master
pull from: docs/ca-docs-writer-v3.8.1-2026-04-05
merge into: cleveragents:master
Conversation 5 Commits 1 Files changed 4 +267
freemo commented 2026-04-05 22:13:31 +00:00
Owner
Copy link
No description provided.
docs: document v3.8.1 changes — skeleton context inheritance, actor YAML-first path, checkpoint triggers
Some checks failed
ci.yml / docs: document v3.8.1 changes — skeleton context inheritance, actor YAML-first path, checkpoint triggers (push) Failing after 0s
Details
ci.yml / docs: document v3.8.1 changes — skeleton context inheritance, actor YAML-first path, checkpoint triggers (pull_request) Failing after 0s
Details
0bb0d57562 
- CHANGELOG.md: add v3.8.1 section documenting ACMS skeleton context
  inheritance (#3563), actor add YAML-first path (#3426), diagnostics
  extended to all 9 providers (#3422), automatic checkpoint triggers
  (#3439), --container-id flag (#2598), ToolLifecycle execute hook
  (#2820), MCPToolResult.data type fix (#2743), automation-profile list
  fix (#2064)
- docs/architecture.md: add Skeleton Context Inheritance subsection under
  Context Management (ACMS) documenting the compression pipeline, key
  parameters, and link to reference doc
- docs/api/actor.md: document ActorRegistry.add() YAML-first persistence
  path introduced in #3426; note deprecation of upsert_actor() for CLI use
- docs/modules/acms-skeleton-context.md: new module doc covering skeleton
  context inheritance — how it works, API parameters, configuration,
  subplan spawning integration, and gotchas
freemo commented 2026-04-05 22:20:19 +00:00
Author
Owner
Copy link

🔍 PR Review — REQUEST CHANGES

Reviewer: ca-pr-self-reviewer | Focus: code-maintainability, readability, documentation | Reason: initial-review

Reviewed documentation-only PR covering v3.8.1 changes across four files: CHANGELOG.md, docs/architecture.md, docs/api/actor.md, and a new docs/modules/acms-skeleton-context.md. The writing quality is generally good, the diagrams are helpful, and the API parameter tables are well-structured. However, I found several issues that must be addressed before merge.

Required Changes

1. 🔗 [BROKEN LINK] docs/modules/acms-skeleton-context.md — Stray period in link path

  • Location: docs/modules/acms-skeleton-context.md, "Relationship to SkeletonCompressorService" section
  • Issue: The link to the skeleton compressor reference doc reads (../reference./skeleton_compressor.md) — note the stray period after reference, making it reference. instead of reference/. This produces a broken link.
  • Required: Change ../reference./skeleton_compressor.md to ../reference/skeleton_compressor.md

2. 📊 [DOC ACCURACY] Contradictory skeleton_ratio default values across documents

  • Locations:
    • docs/architecture.md: Shows SkeletonCompressor (skeleton_ratio=0.15) and table default 0.15
    • docs/modules/acms-skeleton-context.md: Parameter table default 0.15
    • CHANGELOG.md: skeleton_ratio: float = 0.15
    • docs/reference/skeleton_compressor.md: States DEFAULT_SKELETON_RATIO = 0.3 and table shows 0.3 as "Default"
  • Issue: The skeleton compressor reference doc says the default ratio is 0.3, while all other documents in this PR say 0.15. These appear to describe the same parameter being passed to the same compressor, creating a contradiction that will confuse developers.
  • Required: Reconcile the default value across all documents. If the pipeline-level default (0.15) differs from the compressor's standalone default (0.3), this relationship must be explicitly documented — e.g., "The ACMSPipeline passes skeleton_ratio=0.15 to the compressor, overriding the compressor's standalone default of 0.3." Currently the architecture.md diagram labels the compressor itself with 0.15, which directly contradicts the compressor's own reference doc.

3. 📋 [CHANGELOG FORMAT] Miscategorized entries under "Fixed"

  • Location: CHANGELOG.md, [3.8.1] section
  • Issue: Several entries under ### Fixed are feature additions, not bug fixes. Per Keep a Changelog format:
    • "agents diagnostics extended to all 9 providers (#3422)" — This is an enhancement → Added or Changed
    • "Automatic per-tool-write and event-based checkpoint triggers (#3439)" — This is a new feature → Added
    • "--container-id flag on agents resource add container-instance (#2598)" — This is a new flag → Added
    • "ToolLifecycle execute hook added (#2820)" — The word "added" is in the description itself → Added
  • Required: Move these four entries to ### Added (or ### Changed where appropriate). Only MCPToolResult.data type annotation fix (#2743) and automation-profile list output structure fix (#2064) are correctly categorized as Fixed. The actor add YAML-first path (#3426) could arguably be either Changed or Fixed depending on whether the old behavior was considered a bug.

4. 📝 [PROCESS] Missing PR metadata

  • Location: PR metadata
  • Issue: Per CONTRIBUTING.md, every PR must have:
    • Exactly one Type/ label — missing
    • Milestone assignment matching the linked issue — missing
    • Closing keyword in PR body (e.g., Closes #N) — missing
    • ISSUES CLOSED: #N footer in commit message — missing
  • Required: Add Type/Documentation label, assign to the appropriate milestone, and link to the tracking issue (or create one if none exists). The commit message should include the ISSUES CLOSED: footer.

Non-blocking Suggestions

  1. Clarify skeleton_ratio semantics: In docs/modules/acms-skeleton-context.md, the Gotchas section says skeleton_ratio=0.0 "disables compression entirely — all parent fragments pass through unchanged" and skeleton_ratio=1.0 "keeps only the single highest-relevance fragment." But the parameter description says it's a "fraction of budget allocated to skeleton." If ratio=0.0 means 0% of budget goes to skeleton, that would mean no skeleton context (the opposite of "all fragments pass through"). The semantics appear inverted compared to the compressor's own ratio. Consider adding a clarifying note about how the pipeline-level ratio maps to the compressor's behavior.

  2. Layer placement of ACMSPipeline: The import path cleveragents.application.services.acms_service places ACMSPipeline in the Application layer, but the architecture overview places ACMS in the Integration layer. This may be intentional (the service is in Application, the protocol adapter is in Integration), but a brief note would help readers.

Good Aspects

  • ✅ Clean, well-structured writing throughout
  • ✅ Helpful ASCII diagrams in architecture.md and acms-skeleton-context.md
  • ✅ Comprehensive API parameter tables with types and defaults
  • ✅ Good cross-referencing between documents
  • ✅ Gotchas section in acms-skeleton-context.md is valuable
  • ✅ Commit message follows Conventional Changelog format
  • ✅ docs/api/actor.md addition is clean and well-integrated with existing content

Decision: REQUEST CHANGES 🔄

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## 🔍 PR Review — REQUEST CHANGES **Reviewer**: ca-pr-self-reviewer | **Focus**: code-maintainability, readability, documentation | **Reason**: initial-review Reviewed documentation-only PR covering v3.8.1 changes across four files: CHANGELOG.md, docs/architecture.md, docs/api/actor.md, and a new docs/modules/acms-skeleton-context.md. The writing quality is generally good, the diagrams are helpful, and the API parameter tables are well-structured. However, I found several issues that must be addressed before merge. --- ### Required Changes #### 1. 🔗 [BROKEN LINK] `docs/modules/acms-skeleton-context.md` — Stray period in link path - **Location**: `docs/modules/acms-skeleton-context.md`, "Relationship to SkeletonCompressorService" section - **Issue**: The link to the skeleton compressor reference doc reads `(../reference./skeleton_compressor.md)` — note the stray period after `reference`, making it `reference.` instead of `reference/`. This produces a broken link. - **Required**: Change `../reference./skeleton_compressor.md` to `../reference/skeleton_compressor.md` #### 2. 📊 [DOC ACCURACY] Contradictory `skeleton_ratio` default values across documents - **Locations**: - `docs/architecture.md`: Shows `SkeletonCompressor (skeleton_ratio=0.15)` and table default `0.15` - `docs/modules/acms-skeleton-context.md`: Parameter table default `0.15` - `CHANGELOG.md`: `skeleton_ratio: float = 0.15` - `docs/reference/skeleton_compressor.md`: States `DEFAULT_SKELETON_RATIO = 0.3` and table shows `0.3` as "Default" - **Issue**: The skeleton compressor reference doc says the default ratio is **0.3**, while all other documents in this PR say **0.15**. These appear to describe the same parameter being passed to the same compressor, creating a contradiction that will confuse developers. - **Required**: Reconcile the default value across all documents. If the pipeline-level default (0.15) differs from the compressor's standalone default (0.3), this relationship must be explicitly documented — e.g., *"The `ACMSPipeline` passes `skeleton_ratio=0.15` to the compressor, overriding the compressor's standalone default of `0.3`."* Currently the architecture.md diagram labels the compressor itself with 0.15, which directly contradicts the compressor's own reference doc. #### 3. 📋 [CHANGELOG FORMAT] Miscategorized entries under "Fixed" - **Location**: `CHANGELOG.md`, `[3.8.1]` section - **Issue**: Several entries under `### Fixed` are feature additions, not bug fixes. Per [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) format: - `"agents diagnostics extended to all 9 providers (#3422)"` — This is an enhancement → **Added** or **Changed** - `"Automatic per-tool-write and event-based checkpoint triggers (#3439)"` — This is a new feature → **Added** - `"--container-id flag on agents resource add container-instance (#2598)"` — This is a new flag → **Added** - `"ToolLifecycle execute hook added (#2820)"` — The word "added" is in the description itself → **Added** - **Required**: Move these four entries to `### Added` (or `### Changed` where appropriate). Only `MCPToolResult.data type annotation fix (#2743)` and `automation-profile list output structure fix (#2064)` are correctly categorized as Fixed. The `actor add` YAML-first path (#3426) could arguably be either Changed or Fixed depending on whether the old behavior was considered a bug. #### 4. 📝 [PROCESS] Missing PR metadata - **Location**: PR metadata - **Issue**: Per CONTRIBUTING.md, every PR must have: - Exactly one `Type/` label — **missing** - Milestone assignment matching the linked issue — **missing** - Closing keyword in PR body (e.g., `Closes #N`) — **missing** - `ISSUES CLOSED: #N` footer in commit message — **missing** - **Required**: Add `Type/Documentation` label, assign to the appropriate milestone, and link to the tracking issue (or create one if none exists). The commit message should include the `ISSUES CLOSED:` footer. --- ### Non-blocking Suggestions 1. **Clarify `skeleton_ratio` semantics**: In `docs/modules/acms-skeleton-context.md`, the Gotchas section says `skeleton_ratio=0.0` *"disables compression entirely — all parent fragments pass through unchanged"* and `skeleton_ratio=1.0` *"keeps only the single highest-relevance fragment."* But the parameter description says it's a *"fraction of budget allocated to skeleton."* If ratio=0.0 means 0% of budget goes to skeleton, that would mean *no* skeleton context (the opposite of "all fragments pass through"). The semantics appear inverted compared to the compressor's own ratio. Consider adding a clarifying note about how the pipeline-level ratio maps to the compressor's behavior. 2. **Layer placement of `ACMSPipeline`**: The import path `cleveragents.application.services.acms_service` places `ACMSPipeline` in the Application layer, but the architecture overview places ACMS in the Integration layer. This may be intentional (the service is in Application, the protocol adapter is in Integration), but a brief note would help readers. --- ### Good Aspects - ✅ Clean, well-structured writing throughout - ✅ Helpful ASCII diagrams in architecture.md and acms-skeleton-context.md - ✅ Comprehensive API parameter tables with types and defaults - ✅ Good cross-referencing between documents - ✅ Gotchas section in acms-skeleton-context.md is valuable - ✅ Commit message follows Conventional Changelog format - ✅ docs/api/actor.md addition is clean and well-integrated with existing content **Decision: REQUEST CHANGES** 🔄 --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
HAL9000 requested changes 2026-04-08 11:47:31 +00:00
Dismissed
HAL9000 left a comment
Copy link

🔍 PR Review — REQUEST CHANGES

Reviewer: pr-self-reviewer (claude-opus-4-6) | Focus: specification-compliance, requirements-coverage, behavior-correctness | Reason: initial-review

Reviewed documentation-only PR covering v3.8.1 changes across four files: CHANGELOG.md, docs/architecture.md, docs/api/actor.md, and new docs/modules/acms-skeleton-context.md. The writing quality is generally good and the structure is well-organized. However, I found one critical correctness issue and several issues previously flagged that remain unaddressed.

Prior Review Context

A previous review (comment #126311) identified 4 required changes. None appear to have been addressed — the head SHA has not changed since that review. My review confirms those findings and adds a critical new finding about behavioral correctness.

Required Changes

1. 🚨 [CRITICAL — BEHAVIOR CORRECTNESS] skeleton_ratio gotchas describe wrong component's semantics

  • Location: docs/modules/acms-skeleton-context.md, Gotchas section

  • Issue: The gotchas describe the SkeletonCompressorService (standalone service) ratio semantics, NOT the pipeline's skeleton_ratio parameter semantics. These are two fundamentally different parameters:

    Pipeline's skeleton_ratio (what the docs are about):

    • Computes skeleton_budget = int(budget.available_tokens * skeleton_ratio)
    • Higher ratio → more budget for skeleton → more parent context inherited
    • skeleton_ratio=0.0 → skeleton_budget=0 → DepthReductionCompressor returns () → NO parent fragments inherited
    • skeleton_ratio=1.0 → skeleton_budget=available_tokens → maximum parent context inherited

    SkeletonCompressorService's skeleton_ratio (what the gotchas actually describe):

    • Computes budget = original_tokens * (1 - ratio)
    • Higher ratio → more compression → fewer tokens kept
    • skeleton_ratio=0.0 → all fragments pass through
    • skeleton_ratio=1.0 → only top fragment kept

    The documentation says:

    skeleton_ratio=0.0 disables compression entirely — all parent fragments pass through unchanged.
    skeleton_ratio=1.0 keeps only the single highest-relevance fragment.

    Both statements are wrong for the pipeline parameter. I verified against the actual source code:

    • acms_service.py:966: skeleton_budget = int(budget.available_tokens * skeleton_ratio)
    • acms_skeleton_compressor.py:112: if skeleton_budget <= 0 or not fragments: return ()
    • At skeleton_ratio=0.0, the production DepthReductionCompressor returns an empty tuple — zero fragments inherited.

  • Required: Rewrite the gotchas to accurately describe the pipeline-level skeleton_ratio:

    • skeleton_ratio=0.0 → skeleton budget is zero → no parent context inherited (effectively disables skeleton inheritance)
    • skeleton_ratio=1.0 → skeleton budget equals the full available token budget → maximum parent context inherited (may crowd out the child's own context)
    • The parameter description "Fraction of budget.available_tokens allocated to the skeleton" is correct — the gotchas must be consistent with it.

2. 📊 [HIGH — SPECIFICATION COMPLIANCE] Contradictory skeleton_ratio default in reference doc

  • Locations:
    • This PR (all files): skeleton_ratio default = 0.15 ✅ Correct per source code
    • docs/reference/skeleton_compressor.md: DEFAULT_SKELETON_RATIO = 0.3 ❌ Wrong
  • Verification: I checked all source files:
    • skeleton_compressor.py:50: DEFAULT_SKELETON_RATIO: float = 0.15
    • depth_breadth_projection.py:51: DEFAULT_SKELETON_RATIO: float = 0.15
    • acms_service.py:886: skeleton_ratio: float = 0.15
    • acms_pipeline.py:583: skeleton_ratio: float = 0.15
    • project_context.py:68: _DEFAULT_SKELETON_RATIO = 0.15
  • Issue: The reference doc's 0.3 default is a pre-existing error, but since this is a documentation PR specifically covering the skeleton compressor feature, it should fix the contradiction. Developers reading the reference doc will get the wrong default value.
  • Required: Update docs/reference/skeleton_compressor.md to change DEFAULT_SKELETON_RATIO = 0.3 to 0.15 and update the ratio table's "Default" row from 0.3 to 0.15. Also update the table description for 0.3 (currently says "Default") to something like "Moderate" or similar.

3. 🔗 [MEDIUM — BEHAVIOR CORRECTNESS] Broken link — stray period in path

  • Location: docs/modules/acms-skeleton-context.md, "Relationship to SkeletonCompressorService" section
  • Issue: Link reads (../reference./skeleton_compressor.md) — stray period after reference makes it reference. instead of reference/.
  • Required: Change ../reference./skeleton_compressor.md to ../reference/skeleton_compressor.md
  • Note: This was flagged in the previous review and remains unfixed.

4. 📋 [MEDIUM — REQUIREMENTS COVERAGE] CHANGELOG entries miscategorized

  • Location: CHANGELOG.md, [3.8.1] section, ### Fixed subsection
  • Issue: Per Keep a Changelog format, several entries under ### Fixed are feature additions, not bug fixes:
    • agents diagnostics extended to all 9 providers (#3422) → Added (enhancement)
    • Automatic per-tool-write and event-based checkpoint triggers (#3439) → Added (new feature)
    • --container-id flag on agents resource add container-instance (#2598) → Added (new flag)
    • ToolLifecycle execute hook added (#2820) → Added (the word "added" is in the description)
  • Required: Move these four entries to ### Added. Only MCPToolResult.data type annotation fix (#2743) and automation-profile list output structure fix (#2064) are correctly categorized as Fixed.
  • Note: This was flagged in the previous review and remains unfixed.

5. 📝 [LOW — PROCESS] Missing milestone assignment

  • Location: PR metadata
  • Issue: Per CONTRIBUTING.md, PRs must have a milestone assignment. This PR has milestone: null.
  • Required: Assign to the appropriate milestone (likely the v3.8.1 patch milestone if one exists, or the current active milestone).

Non-blocking Observations

  1. Two different SkeletonCompressor abstractions exist: The codebase has both SkeletonCompressorService (standalone, ratio-based) and the SkeletonCompressor protocol (pipeline, budget-based). The documentation should be clearer about which one is being described. The "Relationship to SkeletonCompressorService" section in acms-skeleton-context.md references the service but doesn't explain that the pipeline uses a different interface (SkeletonCompressor protocol with DepthReductionCompressor implementation).

  2. DefaultSkeletonCompressor is a no-op: The default compressor in acms_service.py returns all fragments unchanged regardless of budget. The production DepthReductionCompressor in acms_skeleton_compressor.py is the one that actually compresses. The docs should note this distinction or at least mention that the production compressor is DepthReductionCompressor.

  3. Layer placement note: The import path cleveragents.application.services.acms_service places ACMSPipeline in the Application layer, while the architecture overview places ACMS in the Integration layer. This is likely intentional (service in Application, protocol adapter in Integration), but a brief clarifying note would help readers.

Good Aspects

  • ✅ Clean, well-structured writing throughout all four documents
  • ✅ Helpful ASCII diagrams in architecture.md and acms-skeleton-context.md
  • ✅ Comprehensive API parameter tables with types and defaults
  • ✅ Good cross-referencing between documents
  • ✅ The skeleton_ratio default of 0.15 is correct per source code verification
  • ✅ Type/Documentation label is present
  • ✅ Commit message follows Conventional Changelog format
  • ✅ The SubplanService integration example is valuable for understanding the production path
  • ✅ Gotchas section concept is excellent (just needs corrected content)

Decision: REQUEST CHANGES 🔄

The critical finding (#1) about inverted skeleton_ratio semantics in the gotchas section would cause developers to misunderstand the parameter's behavior, potentially leading to bugs where they set skeleton_ratio=0.0 expecting full parent context inheritance but getting none. This must be fixed before merge.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-self-reviewer

## 🔍 PR Review — REQUEST CHANGES **Reviewer**: pr-self-reviewer (claude-opus-4-6) | **Focus**: specification-compliance, requirements-coverage, behavior-correctness | **Reason**: initial-review Reviewed documentation-only PR covering v3.8.1 changes across four files: CHANGELOG.md, docs/architecture.md, docs/api/actor.md, and new docs/modules/acms-skeleton-context.md. The writing quality is generally good and the structure is well-organized. However, I found **one critical correctness issue** and several issues previously flagged that remain unaddressed. ### Prior Review Context A previous review (comment #126311) identified 4 required changes. **None appear to have been addressed** — the head SHA has not changed since that review. My review confirms those findings and adds a critical new finding about behavioral correctness. --- ### Required Changes #### 1. 🚨 [CRITICAL — BEHAVIOR CORRECTNESS] `skeleton_ratio` gotchas describe wrong component's semantics - **Location**: `docs/modules/acms-skeleton-context.md`, Gotchas section - **Issue**: The gotchas describe the **`SkeletonCompressorService`** (standalone service) ratio semantics, NOT the **pipeline's `skeleton_ratio`** parameter semantics. These are two fundamentally different parameters: **Pipeline's `skeleton_ratio`** (what the docs are about): - Computes `skeleton_budget = int(budget.available_tokens * skeleton_ratio)` - Higher ratio → **more** budget for skeleton → **more** parent context inherited - `skeleton_ratio=0.0` → `skeleton_budget=0` → `DepthReductionCompressor` returns `()` → **NO parent fragments inherited** - `skeleton_ratio=1.0` → `skeleton_budget=available_tokens` → **maximum** parent context inherited **SkeletonCompressorService's `skeleton_ratio`** (what the gotchas actually describe): - Computes `budget = original_tokens * (1 - ratio)` - Higher ratio → **more** compression → **fewer** tokens kept - `skeleton_ratio=0.0` → all fragments pass through - `skeleton_ratio=1.0` → only top fragment kept The documentation says: > `skeleton_ratio=0.0` disables compression entirely — all parent fragments pass through unchanged. > `skeleton_ratio=1.0` keeps only the single highest-relevance fragment. **Both statements are wrong** for the pipeline parameter. I verified against the actual source code: - `acms_service.py:966`: `skeleton_budget = int(budget.available_tokens * skeleton_ratio)` - `acms_skeleton_compressor.py:112`: `if skeleton_budget <= 0 or not fragments: return ()` - At `skeleton_ratio=0.0`, the production `DepthReductionCompressor` returns an **empty tuple** — zero fragments inherited. - **Required**: Rewrite the gotchas to accurately describe the pipeline-level `skeleton_ratio`: - `skeleton_ratio=0.0` → skeleton budget is zero → **no parent context inherited** (effectively disables skeleton inheritance) - `skeleton_ratio=1.0` → skeleton budget equals the full available token budget → **maximum parent context inherited** (may crowd out the child's own context) - The parameter description "Fraction of `budget.available_tokens` allocated to the skeleton" is correct — the gotchas must be consistent with it. #### 2. 📊 [HIGH — SPECIFICATION COMPLIANCE] Contradictory `skeleton_ratio` default in reference doc - **Locations**: - This PR (all files): `skeleton_ratio` default = `0.15` ✅ **Correct per source code** - `docs/reference/skeleton_compressor.md`: `DEFAULT_SKELETON_RATIO = 0.3` ❌ **Wrong** - **Verification**: I checked all source files: - `skeleton_compressor.py:50`: `DEFAULT_SKELETON_RATIO: float = 0.15` - `depth_breadth_projection.py:51`: `DEFAULT_SKELETON_RATIO: float = 0.15` - `acms_service.py:886`: `skeleton_ratio: float = 0.15` - `acms_pipeline.py:583`: `skeleton_ratio: float = 0.15` - `project_context.py:68`: `_DEFAULT_SKELETON_RATIO = 0.15` - **Issue**: The reference doc's `0.3` default is a pre-existing error, but since this is a documentation PR specifically covering the skeleton compressor feature, it should fix the contradiction. Developers reading the reference doc will get the wrong default value. - **Required**: Update `docs/reference/skeleton_compressor.md` to change `DEFAULT_SKELETON_RATIO = 0.3` to `0.15` and update the ratio table's "Default" row from `0.3` to `0.15`. Also update the table description for `0.3` (currently says "Default") to something like "Moderate" or similar. #### 3. 🔗 [MEDIUM — BEHAVIOR CORRECTNESS] Broken link — stray period in path - **Location**: `docs/modules/acms-skeleton-context.md`, "Relationship to SkeletonCompressorService" section - **Issue**: Link reads `(../reference./skeleton_compressor.md)` — stray period after `reference` makes it `reference.` instead of `reference/`. - **Required**: Change `../reference./skeleton_compressor.md` to `../reference/skeleton_compressor.md` - **Note**: This was flagged in the previous review and remains unfixed. #### 4. 📋 [MEDIUM — REQUIREMENTS COVERAGE] CHANGELOG entries miscategorized - **Location**: `CHANGELOG.md`, `[3.8.1]` section, `### Fixed` subsection - **Issue**: Per [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) format, several entries under `### Fixed` are feature additions, not bug fixes: - `agents diagnostics extended to all 9 providers (#3422)` → **Added** (enhancement) - `Automatic per-tool-write and event-based checkpoint triggers (#3439)` → **Added** (new feature) - `--container-id flag on agents resource add container-instance (#2598)` → **Added** (new flag) - `ToolLifecycle execute hook added (#2820)` → **Added** (the word "added" is in the description) - **Required**: Move these four entries to `### Added`. Only `MCPToolResult.data type annotation fix (#2743)` and `automation-profile list output structure fix (#2064)` are correctly categorized as Fixed. - **Note**: This was flagged in the previous review and remains unfixed. #### 5. 📝 [LOW — PROCESS] Missing milestone assignment - **Location**: PR metadata - **Issue**: Per CONTRIBUTING.md, PRs must have a milestone assignment. This PR has `milestone: null`. - **Required**: Assign to the appropriate milestone (likely the v3.8.1 patch milestone if one exists, or the current active milestone). --- ### Non-blocking Observations 1. **Two different `SkeletonCompressor` abstractions exist**: The codebase has both `SkeletonCompressorService` (standalone, ratio-based) and the `SkeletonCompressor` protocol (pipeline, budget-based). The documentation should be clearer about which one is being described. The "Relationship to SkeletonCompressorService" section in acms-skeleton-context.md references the service but doesn't explain that the pipeline uses a different interface (`SkeletonCompressor` protocol with `DepthReductionCompressor` implementation). 2. **`DefaultSkeletonCompressor` is a no-op**: The default compressor in `acms_service.py` returns all fragments unchanged regardless of budget. The production `DepthReductionCompressor` in `acms_skeleton_compressor.py` is the one that actually compresses. The docs should note this distinction or at least mention that the production compressor is `DepthReductionCompressor`. 3. **Layer placement note**: The import path `cleveragents.application.services.acms_service` places `ACMSPipeline` in the Application layer, while the architecture overview places ACMS in the Integration layer. This is likely intentional (service in Application, protocol adapter in Integration), but a brief clarifying note would help readers. --- ### Good Aspects - ✅ Clean, well-structured writing throughout all four documents - ✅ Helpful ASCII diagrams in architecture.md and acms-skeleton-context.md - ✅ Comprehensive API parameter tables with types and defaults - ✅ Good cross-referencing between documents - ✅ The `skeleton_ratio` default of `0.15` is correct per source code verification - ✅ `Type/Documentation` label is present - ✅ Commit message follows Conventional Changelog format - ✅ The SubplanService integration example is valuable for understanding the production path - ✅ Gotchas section concept is excellent (just needs corrected content) **Decision: REQUEST CHANGES** 🔄 The critical finding (#1) about inverted `skeleton_ratio` semantics in the gotchas section would cause developers to misunderstand the parameter's behavior, potentially leading to bugs where they set `skeleton_ratio=0.0` expecting full parent context inheritance but getting none. This must be fixed before merge. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: pr-self-reviewer
HAL9000 added this to the v3.4.0 milestone 2026-04-08 16:26:06 +00:00
HAL9000 requested changes 2026-04-09 00:30:53 +00:00
Dismissed
HAL9000 left a comment
Copy link

🔍 PR Review — REQUEST CHANGES

Reviewer: pr-self-reviewer (claude-sonnet-4-6) | Focus: security-concerns, input-validation, access-control | Reason: stale-review (4+ days old, no changes since prior reviews)

Reviewed documentation-only PR covering v3.8.1 changes across four files: CHANGELOG.md, docs/architecture.md, docs/api/actor.md, and new docs/modules/acms-skeleton-context.md.

Prior Review Context

Two previous reviews have requested changes:

  • Comment #126311 (2026-04-05): Identified 4 required changes
  • HAL9000 formal review (2026-04-08): Confirmed all 4 prior issues + added a critical behavioral correctness finding

The head SHA (0bb0d57562f03c4663111b55acb3664d07843eae) has not changed since the PR was opened. None of the requested changes have been addressed. This review confirms all prior findings and adds a new critical finding.

Required Changes

1. 🚨 [CRITICAL — WRONG ISSUE CLOSED] Closes #3695 links to an unrelated issue

  • Location: PR description body
  • Issue: The PR body contains Closes #3695, but issue #3695 is titled "TEST-INFRA: [flaky-tests] Replace time.sleep() with time mocking in memory_service tests" — a completely unrelated test infrastructure issue about replacing time.sleep() calls with mocked clocks in features/steps/memory_service_coverage_steps.py. This documentation PR has nothing to do with that issue.
  • Impact: If this PR is merged as-is, it will incorrectly close a legitimate open bug/test-infra issue that has not been resolved. The flaky-test issue will be silently marked as done when the underlying problem still exists.
  • Required: Either:
    • (a) Correct the closing keyword to reference the actual documentation tracking issue (if one exists), or
    • (b) Remove the Closes #3695 line entirely and create a proper documentation tracking issue to link to, or
    • (c) If this PR was intentionally created to close #3695 (which seems unlikely given the content mismatch), explain the relationship in the PR description.

2. 🚨 [CRITICAL — BEHAVIOR CORRECTNESS] Inverted skeleton_ratio semantics in Gotchas section

  • Location: docs/modules/acms-skeleton-context.md, Gotchas section

  • Issue (confirmed by HAL9000's review with source code verification): The gotchas describe the SkeletonCompressorService standalone semantics, NOT the pipeline's skeleton_ratio parameter semantics. The current text states:

    skeleton_ratio=0.0 disables compression entirely — all parent fragments pass through unchanged.
    skeleton_ratio=1.0 keeps only the single highest-relevance fragment.

    Both statements are wrong for the pipeline parameter. The pipeline computes skeleton_budget = int(budget.available_tokens * skeleton_ratio):

    • skeleton_ratio=0.0 → skeleton_budget=0 → DepthReductionCompressor returns an empty tuple → zero parent fragments inherited (not "all pass through")
    • skeleton_ratio=1.0 → skeleton_budget=available_tokens → maximum parent context inherited (not "only one fragment")

    This is the opposite of what the documentation claims. Developers following this documentation will set skeleton_ratio=0.0 expecting full parent context inheritance but will get no parent context at all.

  • Required: Rewrite the gotchas to accurately describe the pipeline-level parameter:

    • skeleton_ratio=0.0 → skeleton budget is zero → no parent context inherited (effectively disables skeleton inheritance)
    • skeleton_ratio=1.0 → skeleton budget equals the full available token budget → maximum parent context inherited (may crowd out the child's own context)

  • Note: This was flagged in the previous review and remains unfixed.

3. 🔗 [MEDIUM — BROKEN LINK] Stray period in link path

  • Location: docs/modules/acms-skeleton-context.md, "Relationship to SkeletonCompressorService" section
  • Issue: The link reads (../reference./skeleton_compressor.md) — note the stray period after reference, making it reference. instead of reference/. This produces a broken link.
  • Required: Change ../reference./skeleton_compressor.md to ../reference/skeleton_compressor.md
  • Note: This was flagged in both prior reviews and remains unfixed.

4. 📋 [MEDIUM — CHANGELOG FORMAT] Entries miscategorized under ### Fixed

  • Location: CHANGELOG.md, [3.8.1] section
  • Issue: Per Keep a Changelog format, the following entries under ### Fixed are feature additions, not bug fixes:
    • agents diagnostics extended to all 9 providers (#3422) → Added (enhancement)
    • Automatic per-tool-write and event-based checkpoint triggers (#3439) → Added (new feature)
    • --container-id flag on agents resource add container-instance (#2598) → Added (new flag)
    • ToolLifecycle execute hook added (#2820) → Added (the word "added" is in the description itself)
  • Required: Move these four entries to ### Added. Only MCPToolResult.data type annotation fix (#2743) and automation-profile list output structure fix (#2064) are correctly categorized as Fixed.
  • Note: This was flagged in both prior reviews and remains unfixed.

5. 📝 [LOW — PROCESS] Milestone mismatch

  • Location: PR metadata
  • Issue: This PR documents v3.8.1 changes but is assigned to the v3.4.0 milestone. These are very different milestones — v3.4.0 is "M5: ACMS v1 + Context Scaling" and is currently open with 56 open issues. Assigning a v3.8.1 documentation PR to v3.4.0 pollutes that milestone's tracking.
  • Required: Assign to the appropriate v3.8.x milestone (or the current active milestone if v3.8.1 doesn't have its own milestone).

Security-Focused Analysis (Primary Focus Areas)

Given the focus on security-concerns, input-validation, and access-control, I performed a targeted review of the documentation content:

✅ No sensitive information leaks detected: No API keys, credentials, internal hostnames, or proprietary paths are exposed in any of the four documentation files.

✅ Security mechanisms correctly documented: docs/architecture.md correctly documents:

  • SandboxPathRewriter prevents path traversal (MCP Integration section)
  • Template security via ADR-032: Sandboxed Jinja2 with allowlist (Key Design Decisions table)
  • Safety profiles controlling sandbox requirements, human approval gates, and cost limits (Tool System section)

✅ Access control defaults are secure: docs/api/actor.md documents ActorRegistry.add() with update=False as the default, which is a secure default preventing accidental overwrites of existing actor configurations.

✅ Input validation documented: docs/api/actor.md correctly notes that ActorLoader validates graph reachability and reports YAML line/column positions on error.

⚠️ Minor observation: The docs/api/actor.md note about upsert_actor() being deprecated but "still available for programmatic use" could benefit from a security note clarifying whether upsert_actor() bypasses any validation that add() performs. If upsert_actor() skips schema version checks or compiled metadata preservation, callers using the legacy path could inadvertently store malformed actor configurations. This is non-blocking but worth considering.

Good Aspects

  • ✅ Clean, well-structured writing throughout all four documents
  • ✅ Helpful ASCII diagrams in architecture.md and acms-skeleton-context.md
  • ✅ Comprehensive API parameter tables with types and defaults
  • ✅ Good cross-referencing between documents
  • ✅ The skeleton_ratio default of 0.15 is correct per source code
  • ✅ Type/Documentation label is present
  • ✅ Commit message follows Conventional Changelog format
  • ✅ The SubplanService integration example is valuable for understanding the production path
  • ✅ Gotchas section concept is excellent (just needs corrected content)
  • ✅ No security-sensitive information exposed in documentation

Decision: REQUEST CHANGES 🔄

The PR has five issues requiring fixes, two of which are critical:

  1. The Closes #3695 link will incorrectly close an unrelated flaky-test issue
  2. The inverted skeleton_ratio semantics in the Gotchas section will cause developers to misuse the API

Additionally, this is the third review requesting changes with no response from the author. The PR is 4+ days old and stale. Please address all required changes and push an updated commit.

Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-self-reviewer

## 🔍 PR Review — REQUEST CHANGES **Reviewer**: pr-self-reviewer (claude-sonnet-4-6) | **Focus**: security-concerns, input-validation, access-control | **Reason**: stale-review (4+ days old, no changes since prior reviews) Reviewed documentation-only PR covering v3.8.1 changes across four files: CHANGELOG.md, docs/architecture.md, docs/api/actor.md, and new docs/modules/acms-skeleton-context.md. ### Prior Review Context Two previous reviews have requested changes: - **Comment #126311** (2026-04-05): Identified 4 required changes - **HAL9000 formal review** (2026-04-08): Confirmed all 4 prior issues + added a critical behavioral correctness finding **The head SHA (`0bb0d57562f03c4663111b55acb3664d07843eae`) has not changed since the PR was opened.** None of the requested changes have been addressed. This review confirms all prior findings and adds a new critical finding. --- ### Required Changes #### 1. 🚨 [CRITICAL — WRONG ISSUE CLOSED] `Closes #3695` links to an unrelated issue - **Location**: PR description body - **Issue**: The PR body contains `Closes #3695`, but issue #3695 is titled **"TEST-INFRA: [flaky-tests] Replace time.sleep() with time mocking in memory_service tests"** — a completely unrelated test infrastructure issue about replacing `time.sleep()` calls with mocked clocks in `features/steps/memory_service_coverage_steps.py`. This documentation PR has nothing to do with that issue. - **Impact**: If this PR is merged as-is, it will **incorrectly close a legitimate open bug/test-infra issue** that has not been resolved. The flaky-test issue will be silently marked as done when the underlying problem still exists. - **Required**: Either: - (a) Correct the closing keyword to reference the actual documentation tracking issue (if one exists), or - (b) Remove the `Closes #3695` line entirely and create a proper documentation tracking issue to link to, or - (c) If this PR was intentionally created to close #3695 (which seems unlikely given the content mismatch), explain the relationship in the PR description. #### 2. 🚨 [CRITICAL — BEHAVIOR CORRECTNESS] Inverted `skeleton_ratio` semantics in Gotchas section - **Location**: `docs/modules/acms-skeleton-context.md`, Gotchas section - **Issue** (confirmed by HAL9000's review with source code verification): The gotchas describe the **`SkeletonCompressorService`** standalone semantics, NOT the pipeline's `skeleton_ratio` parameter semantics. The current text states: > `skeleton_ratio=0.0` disables compression entirely — all parent fragments pass through unchanged. > `skeleton_ratio=1.0` keeps only the single highest-relevance fragment. **Both statements are wrong** for the pipeline parameter. The pipeline computes `skeleton_budget = int(budget.available_tokens * skeleton_ratio)`: - `skeleton_ratio=0.0` → `skeleton_budget=0` → `DepthReductionCompressor` returns an **empty tuple** → **zero parent fragments inherited** (not "all pass through") - `skeleton_ratio=1.0` → `skeleton_budget=available_tokens` → **maximum parent context inherited** (not "only one fragment") This is the opposite of what the documentation claims. Developers following this documentation will set `skeleton_ratio=0.0` expecting full parent context inheritance but will get **no** parent context at all. - **Required**: Rewrite the gotchas to accurately describe the pipeline-level parameter: - `skeleton_ratio=0.0` → skeleton budget is zero → **no parent context inherited** (effectively disables skeleton inheritance) - `skeleton_ratio=1.0` → skeleton budget equals the full available token budget → **maximum parent context inherited** (may crowd out the child's own context) - **Note**: This was flagged in the previous review and remains unfixed. #### 3. 🔗 [MEDIUM — BROKEN LINK] Stray period in link path - **Location**: `docs/modules/acms-skeleton-context.md`, "Relationship to SkeletonCompressorService" section - **Issue**: The link reads `(../reference./skeleton_compressor.md)` — note the stray period after `reference`, making it `reference.` instead of `reference/`. This produces a broken link. - **Required**: Change `../reference./skeleton_compressor.md` to `../reference/skeleton_compressor.md` - **Note**: This was flagged in both prior reviews and remains unfixed. #### 4. 📋 [MEDIUM — CHANGELOG FORMAT] Entries miscategorized under `### Fixed` - **Location**: `CHANGELOG.md`, `[3.8.1]` section - **Issue**: Per [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) format, the following entries under `### Fixed` are feature additions, not bug fixes: - `agents diagnostics extended to all 9 providers (#3422)` → **Added** (enhancement) - `Automatic per-tool-write and event-based checkpoint triggers (#3439)` → **Added** (new feature) - `--container-id flag on agents resource add container-instance (#2598)` → **Added** (new flag) - `ToolLifecycle execute hook added (#2820)` → **Added** (the word "added" is in the description itself) - **Required**: Move these four entries to `### Added`. Only `MCPToolResult.data type annotation fix (#2743)` and `automation-profile list output structure fix (#2064)` are correctly categorized as Fixed. - **Note**: This was flagged in both prior reviews and remains unfixed. #### 5. 📝 [LOW — PROCESS] Milestone mismatch - **Location**: PR metadata - **Issue**: This PR documents v3.8.1 changes but is assigned to the **v3.4.0** milestone. These are very different milestones — v3.4.0 is "M5: ACMS v1 + Context Scaling" and is currently open with 56 open issues. Assigning a v3.8.1 documentation PR to v3.4.0 pollutes that milestone's tracking. - **Required**: Assign to the appropriate v3.8.x milestone (or the current active milestone if v3.8.1 doesn't have its own milestone). --- ### Security-Focused Analysis (Primary Focus Areas) Given the focus on **security-concerns**, **input-validation**, and **access-control**, I performed a targeted review of the documentation content: **✅ No sensitive information leaks detected**: No API keys, credentials, internal hostnames, or proprietary paths are exposed in any of the four documentation files. **✅ Security mechanisms correctly documented**: `docs/architecture.md` correctly documents: - `SandboxPathRewriter` prevents path traversal (MCP Integration section) - Template security via ADR-032: Sandboxed Jinja2 with allowlist (Key Design Decisions table) - Safety profiles controlling sandbox requirements, human approval gates, and cost limits (Tool System section) **✅ Access control defaults are secure**: `docs/api/actor.md` documents `ActorRegistry.add()` with `update=False` as the default, which is a secure default preventing accidental overwrites of existing actor configurations. **✅ Input validation documented**: `docs/api/actor.md` correctly notes that `ActorLoader` validates graph reachability and reports YAML line/column positions on error. **⚠️ Minor observation**: The `docs/api/actor.md` note about `upsert_actor()` being deprecated but "still available for programmatic use" could benefit from a security note clarifying whether `upsert_actor()` bypasses any validation that `add()` performs. If `upsert_actor()` skips schema version checks or compiled metadata preservation, callers using the legacy path could inadvertently store malformed actor configurations. This is non-blocking but worth considering. --- ### Good Aspects - ✅ Clean, well-structured writing throughout all four documents - ✅ Helpful ASCII diagrams in architecture.md and acms-skeleton-context.md - ✅ Comprehensive API parameter tables with types and defaults - ✅ Good cross-referencing between documents - ✅ The `skeleton_ratio` default of `0.15` is correct per source code - ✅ `Type/Documentation` label is present - ✅ Commit message follows Conventional Changelog format - ✅ The SubplanService integration example is valuable for understanding the production path - ✅ Gotchas section concept is excellent (just needs corrected content) - ✅ No security-sensitive information exposed in documentation --- **Decision: REQUEST CHANGES** 🔄 The PR has five issues requiring fixes, two of which are critical: 1. The `Closes #3695` link will incorrectly close an unrelated flaky-test issue 2. The inverted `skeleton_ratio` semantics in the Gotchas section will cause developers to misuse the API Additionally, this is the **third review requesting changes** with no response from the author. The PR is 4+ days old and stale. Please address all required changes and push an updated commit. --- **Automated by CleverAgents Bot** Supervisor: PR Review Pool | Agent: pr-self-reviewer
HAL9000 commented 2026-04-09 05:43:55 +00:00
Owner
Copy link

Label compliance fix applied:

  • Added missing label(s) to bring PR into compliance with CONTRIBUTING.md
  • PRs should have Type/, State/, and Priority/* labels matching their linked issue

Automated by CleverAgents Bot
Supervisor: Backlog Grooming | Agent: backlog-groomer

Label compliance fix applied: - Added missing label(s) to bring PR into compliance with CONTRIBUTING.md - PRs should have Type/*, State/*, and Priority/* labels matching their linked issue --- **Automated by CleverAgents Bot** Supervisor: Backlog Grooming | Agent: backlog-groomer
HAL9000 requested changes 2026-04-09 06:52:51 +00:00
HAL9000 left a comment
Copy link

🔍 PR Review — REQUEST CHANGES

Reviewer: pr-self-reviewer (claude-sonnet-4-6) | Focus: error-handling-patterns, edge-cases, boundary-conditions | Reason: stale-review (4+ days, head SHA 0bb0d57562f03c4663111b55acb3664d07843eae unchanged since PR opened)

Reviewed documentation-only PR covering v3.8.1 changes across four files: CHANGELOG.md, docs/architecture.md, docs/api/actor.md, and new docs/modules/acms-skeleton-context.md.

Prior Review Context

Three previous reviews have requested changes:

  • Comment #126311 (2026-04-05): 4 required changes identified
  • HAL9000 formal review #4248 (2026-04-08): Confirmed all 4 + added critical behavioral correctness finding
  • HAL9000 formal review #4417 (2026-04-09): Confirmed all prior issues + added wrong-issue-closed finding

The head SHA has not changed since the PR was opened. None of the requested changes have been addressed.

Required Changes

1. 🚨 [CRITICAL — BOUNDARY CONDITIONS WRONG] Inverted skeleton_ratio semantics in Gotchas section

  • Location: docs/modules/acms-skeleton-context.md, Gotchas section

  • Issue: This is directly relevant to my focus on boundary-conditions. The Gotchas section documents the two boundary values (0.0 and 1.0) with inverted semantics that describe the standalone SkeletonCompressorService, not the pipeline's skeleton_ratio parameter.

    Current (wrong) text:

    skeleton_ratio=0.0 disables compression entirely — all parent fragments pass through unchanged. Use this only when you want the full parent context in the child (rare; usually exceeds budget).
    skeleton_ratio=1.0 keeps only the single highest-relevance fragment. Useful for very tight token budgets.

    Actual pipeline behaviour (verified against source code by prior reviewers):

    • The pipeline computes skeleton_budget = int(budget.available_tokens * skeleton_ratio) (acms_service.py:966)
    • skeleton_ratio=0.0 → skeleton_budget = 0 → DepthReductionCompressor returns () → zero parent fragments inherited (not "all pass through")
    • skeleton_ratio=1.0 → skeleton_budget = available_tokens → maximum parent context inherited (not "only one fragment")

    The semantics are the opposite of what is documented. A developer following this documentation who sets skeleton_ratio=0.0 expecting full parent context inheritance will get no parent context at all — a silent, hard-to-debug failure.

  • Required: Rewrite the boundary-condition gotchas to accurately describe the pipeline parameter:

    • skeleton_ratio=0.0 → skeleton budget is zero → no parent context inherited (effectively disables skeleton inheritance entirely)
    • skeleton_ratio=1.0 → skeleton budget equals the full available token budget → maximum parent context inherited (may crowd out the child's own context)

  • Note: Flagged in two prior reviews. Remains unfixed.

2. 🚨 [CRITICAL — EDGE CASE DOCUMENTATION] docs/reference/skeleton_compressor.md contradicts all PR documents on default value

  • Location: docs/reference/skeleton_compressor.md (on master), vs. all four files in this PR

  • Issue: This is directly relevant to my focus on edge-cases. The reference doc on master states DEFAULT_SKELETON_RATIO = 0.3 and the table shows 0.3 as "Default". All four files in this PR consistently use 0.15 as the default. I verified the reference doc on master — it still shows 0.3.

    Since this PR specifically documents the skeleton context inheritance feature, it should fix this pre-existing contradiction. Developers reading docs/reference/skeleton_compressor.md will get the wrong default value (0.3) while the pipeline actually uses 0.15.

  • Required: Update docs/reference/skeleton_compressor.md to:

    • Change DEFAULT_SKELETON_RATIO = 0.3 to DEFAULT_SKELETON_RATIO = 0.15
    • Update the table row for 0.3 from "Default" to "Moderate" (or similar)
    • Add a row for 0.15 as "Default"

  • Note: Flagged in prior review. Remains unfixed.

3. 🔗 [HIGH — BROKEN LINK] Stray period in link path

  • Location: docs/modules/acms-skeleton-context.md, "Relationship to SkeletonCompressorService" section
  • Issue: The link to the skeleton compressor reference doc contains a stray period: (../reference./skeleton_compressor.md) — the reference. should be reference/. This produces a broken link.
  • Required: Change ../reference./skeleton_compressor.md to ../reference/skeleton_compressor.md
  • Note: Flagged in all three prior reviews. Remains unfixed.

4. 📋 [MEDIUM — CHANGELOG FORMAT] Feature additions miscategorized under ### Fixed

  • Location: CHANGELOG.md, [3.8.1] section, ### Fixed subsection
  • Issue: Per Keep a Changelog format, the following entries are feature additions, not bug fixes:
    • agents diagnostics extended to all 9 providers (#3422) → Added (enhancement)
    • Automatic per-tool-write and event-based checkpoint triggers (#3439) → Added (new feature)
    • --container-id flag on agents resource add container-instance (#2598) → Added (new flag)
    • ToolLifecycle execute hook added (#2820) → Added (the word "added" is in the description itself)
  • Required: Move these four entries to ### Added. Only MCPToolResult.data type annotation fix (#2743) and automation-profile list output structure fix (#2064) are correctly categorized as Fixed.
  • Note: Flagged in all three prior reviews. Remains unfixed.

5. 📝 [MEDIUM — PROCESS] Missing closing keyword in PR body

  • Location: PR body (currently empty "")
  • Issue: Per CONTRIBUTING.md, every PR must have a closing keyword (Closes #N or Fixes #N) in the PR body linking to the tracked issue. The PR body is currently empty.
    • The previous review (id:4417) noted a Closes #3695 was present, but that linked to an unrelated flaky-test issue. The current PR body is empty — the closing keyword has been removed but not replaced with a correct one.
  • Required: Add a Closes #N line referencing the actual documentation tracking issue for this work. If no tracking issue exists, create one first.

6. 📅 [LOW — PROCESS] Milestone mismatch

  • Location: PR metadata
  • Issue: This PR documents v3.8.1 changes but is assigned to the v3.4.0 milestone ("M5: ACMS v1 + Context Scaling"). These are very different milestones. Assigning a v3.8.1 documentation PR to v3.4.0 pollutes that milestone's tracking.
  • Required: Assign to the appropriate v3.8.x milestone (or the current active milestone if v3.8.1 doesn't have its own).
  • Note: Flagged in prior review. Remains unfixed.

Focus Area Analysis: Error Handling, Edge Cases, Boundary Conditions

Given the focus on error-handling-patterns, edge-cases, and boundary-conditions, I performed targeted analysis of the documentation content:

❌ Boundary conditions documented incorrectly (Issue #1 above):
The skeleton_ratio boundary values 0.0 and 1.0 are the most important edge cases for this parameter, and both are documented with inverted semantics. This is the most critical finding.

✅ Error handling documented correctly in docs/reference/skeleton_compressor.md:

  • ValueError raised for skeleton_ratio outside [0.0, 1.0] — correctly documented
  • Input validation step is listed first in the compression algorithm

✅ None edge case handled in docs/modules/acms-skeleton-context.md:

  • parent_fragments=None correctly documented as disabling skeleton compression
  • ContextPayload.skeleton_fragments correctly documented as empty tuple when no parent context provided

✅ Budget edge case partially documented:

  • The gotcha about budget.available_tokens vs budget.max_tokens is correct and valuable
  • The gotcha about no deduplication between skeleton and child fragments is correct and valuable

⚠️ Missing edge case: What happens when parent_fragments is an empty tuple ()? The docs cover None but not (). Is the behaviour the same? This is a non-blocking observation but worth clarifying.

✅ docs/api/actor.md error handling:

  • Compilation error table is comprehensive and well-structured
  • update=False default is a secure, fail-safe default (prevents accidental overwrites)
  • YAML line/column error reporting is documented

Good Aspects

  • ✅ Clean, well-structured writing throughout all four documents
  • ✅ Helpful ASCII diagrams in architecture.md and acms-skeleton-context.md
  • ✅ Comprehensive API parameter tables with types and defaults
  • ✅ Good cross-referencing between documents
  • ✅ The skeleton_ratio default of 0.15 is correct per source code
  • ✅ Type/Documentation label is present
  • ✅ Commit message follows Conventional Changelog format
  • ✅ The SubplanService integration example is valuable for understanding the production path
  • ✅ Gotchas section concept is excellent — the non-boundary-condition gotchas are correct and valuable
  • ✅ docs/api/actor.md is clean, well-integrated, and accurately documents the YAML-first path

Decision: REQUEST CHANGES 🔄

This is the fourth review requesting changes on a PR that has been open for 4+ days with zero commits. The two critical issues (inverted boundary-condition semantics and the reference doc default value contradiction) will cause real developer confusion and potential bugs. The broken link has been flagged in every review. Please address all required changes and push an updated commit.

Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-self-reviewer

## 🔍 PR Review — REQUEST CHANGES **Reviewer**: pr-self-reviewer (claude-sonnet-4-6) | **Focus**: error-handling-patterns, edge-cases, boundary-conditions | **Reason**: stale-review (4+ days, head SHA `0bb0d57562f03c4663111b55acb3664d07843eae` unchanged since PR opened) Reviewed documentation-only PR covering v3.8.1 changes across four files: `CHANGELOG.md`, `docs/architecture.md`, `docs/api/actor.md`, and new `docs/modules/acms-skeleton-context.md`. ### Prior Review Context Three previous reviews have requested changes: - **Comment #126311** (2026-04-05): 4 required changes identified - **HAL9000 formal review #4248** (2026-04-08): Confirmed all 4 + added critical behavioral correctness finding - **HAL9000 formal review #4417** (2026-04-09): Confirmed all prior issues + added wrong-issue-closed finding **The head SHA has not changed since the PR was opened. None of the requested changes have been addressed.** --- ### Required Changes #### 1. 🚨 [CRITICAL — BOUNDARY CONDITIONS WRONG] Inverted `skeleton_ratio` semantics in Gotchas section - **Location**: `docs/modules/acms-skeleton-context.md`, Gotchas section - **Issue**: This is directly relevant to my focus on **boundary-conditions**. The Gotchas section documents the two boundary values (`0.0` and `1.0`) with **inverted semantics** that describe the standalone `SkeletonCompressorService`, not the pipeline's `skeleton_ratio` parameter. **Current (wrong) text:** > `skeleton_ratio=0.0` disables compression entirely — all parent fragments pass through unchanged. Use this only when you want the full parent context in the child (rare; usually exceeds budget). > `skeleton_ratio=1.0` keeps only the single highest-relevance fragment. Useful for very tight token budgets. **Actual pipeline behaviour** (verified against source code by prior reviewers): - The pipeline computes `skeleton_budget = int(budget.available_tokens * skeleton_ratio)` (`acms_service.py:966`) - `skeleton_ratio=0.0` → `skeleton_budget = 0` → `DepthReductionCompressor` returns `()` → **zero parent fragments inherited** (not "all pass through") - `skeleton_ratio=1.0` → `skeleton_budget = available_tokens` → **maximum parent context inherited** (not "only one fragment") The semantics are the **opposite** of what is documented. A developer following this documentation who sets `skeleton_ratio=0.0` expecting full parent context inheritance will get **no** parent context at all — a silent, hard-to-debug failure. - **Required**: Rewrite the boundary-condition gotchas to accurately describe the pipeline parameter: - `skeleton_ratio=0.0` → skeleton budget is zero → **no parent context inherited** (effectively disables skeleton inheritance entirely) - `skeleton_ratio=1.0` → skeleton budget equals the full available token budget → **maximum parent context inherited** (may crowd out the child's own context) - **Note**: Flagged in two prior reviews. Remains unfixed. #### 2. 🚨 [CRITICAL — EDGE CASE DOCUMENTATION] `docs/reference/skeleton_compressor.md` contradicts all PR documents on default value - **Location**: `docs/reference/skeleton_compressor.md` (on master), vs. all four files in this PR - **Issue**: This is directly relevant to my focus on **edge-cases**. The reference doc on master states `DEFAULT_SKELETON_RATIO = 0.3` and the table shows `0.3` as "Default". All four files in this PR consistently use `0.15` as the default. I verified the reference doc on master — it still shows `0.3`. Since this PR specifically documents the skeleton context inheritance feature, it should fix this pre-existing contradiction. Developers reading `docs/reference/skeleton_compressor.md` will get the wrong default value (`0.3`) while the pipeline actually uses `0.15`. - **Required**: Update `docs/reference/skeleton_compressor.md` to: - Change `DEFAULT_SKELETON_RATIO = 0.3` to `DEFAULT_SKELETON_RATIO = 0.15` - Update the table row for `0.3` from "Default" to "Moderate" (or similar) - Add a row for `0.15` as "Default" - **Note**: Flagged in prior review. Remains unfixed. #### 3. 🔗 [HIGH — BROKEN LINK] Stray period in link path - **Location**: `docs/modules/acms-skeleton-context.md`, "Relationship to SkeletonCompressorService" section - **Issue**: The link to the skeleton compressor reference doc contains a stray period: `(../reference./skeleton_compressor.md)` — the `reference.` should be `reference/`. This produces a broken link. - **Required**: Change `../reference./skeleton_compressor.md` to `../reference/skeleton_compressor.md` - **Note**: Flagged in all three prior reviews. Remains unfixed. #### 4. 📋 [MEDIUM — CHANGELOG FORMAT] Feature additions miscategorized under `### Fixed` - **Location**: `CHANGELOG.md`, `[3.8.1]` section, `### Fixed` subsection - **Issue**: Per [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) format, the following entries are feature additions, not bug fixes: - `agents diagnostics extended to all 9 providers (#3422)` → **Added** (enhancement) - `Automatic per-tool-write and event-based checkpoint triggers (#3439)` → **Added** (new feature) - `--container-id flag on agents resource add container-instance (#2598)` → **Added** (new flag) - `ToolLifecycle execute hook added (#2820)` → **Added** (the word "added" is in the description itself) - **Required**: Move these four entries to `### Added`. Only `MCPToolResult.data type annotation fix (#2743)` and `automation-profile list output structure fix (#2064)` are correctly categorized as Fixed. - **Note**: Flagged in all three prior reviews. Remains unfixed. #### 5. 📝 [MEDIUM — PROCESS] Missing closing keyword in PR body - **Location**: PR body (currently empty `""`) - **Issue**: Per CONTRIBUTING.md, every PR must have a closing keyword (`Closes #N` or `Fixes #N`) in the PR body linking to the tracked issue. The PR body is currently empty. - The previous review (id:4417) noted a `Closes #3695` was present, but that linked to an **unrelated** flaky-test issue. The current PR body is empty — the closing keyword has been removed but not replaced with a correct one. - **Required**: Add a `Closes #N` line referencing the actual documentation tracking issue for this work. If no tracking issue exists, create one first. #### 6. 📅 [LOW — PROCESS] Milestone mismatch - **Location**: PR metadata - **Issue**: This PR documents v3.8.1 changes but is assigned to the **v3.4.0** milestone ("M5: ACMS v1 + Context Scaling"). These are very different milestones. Assigning a v3.8.1 documentation PR to v3.4.0 pollutes that milestone's tracking. - **Required**: Assign to the appropriate v3.8.x milestone (or the current active milestone if v3.8.1 doesn't have its own). - **Note**: Flagged in prior review. Remains unfixed. --- ### Focus Area Analysis: Error Handling, Edge Cases, Boundary Conditions Given the focus on **error-handling-patterns**, **edge-cases**, and **boundary-conditions**, I performed targeted analysis of the documentation content: **❌ Boundary conditions documented incorrectly** (Issue #1 above): The `skeleton_ratio` boundary values `0.0` and `1.0` are the most important edge cases for this parameter, and both are documented with inverted semantics. This is the most critical finding. **✅ Error handling documented correctly** in `docs/reference/skeleton_compressor.md`: - `ValueError` raised for `skeleton_ratio` outside `[0.0, 1.0]` — correctly documented - Input validation step is listed first in the compression algorithm **✅ `None` edge case handled** in `docs/modules/acms-skeleton-context.md`: - `parent_fragments=None` correctly documented as disabling skeleton compression - `ContextPayload.skeleton_fragments` correctly documented as empty tuple when no parent context provided **✅ Budget edge case partially documented**: - The gotcha about `budget.available_tokens` vs `budget.max_tokens` is correct and valuable - The gotcha about no deduplication between skeleton and child fragments is correct and valuable **⚠️ Missing edge case**: What happens when `parent_fragments` is an empty tuple `()`? The docs cover `None` but not `()`. Is the behaviour the same? This is a non-blocking observation but worth clarifying. **✅ `docs/api/actor.md` error handling**: - Compilation error table is comprehensive and well-structured - `update=False` default is a secure, fail-safe default (prevents accidental overwrites) - YAML line/column error reporting is documented --- ### Good Aspects - ✅ Clean, well-structured writing throughout all four documents - ✅ Helpful ASCII diagrams in `architecture.md` and `acms-skeleton-context.md` - ✅ Comprehensive API parameter tables with types and defaults - ✅ Good cross-referencing between documents - ✅ The `skeleton_ratio` default of `0.15` is correct per source code - ✅ `Type/Documentation` label is present - ✅ Commit message follows Conventional Changelog format - ✅ The SubplanService integration example is valuable for understanding the production path - ✅ Gotchas section concept is excellent — the non-boundary-condition gotchas are correct and valuable - ✅ `docs/api/actor.md` is clean, well-integrated, and accurately documents the YAML-first path --- **Decision: REQUEST CHANGES** 🔄 This is the **fourth review requesting changes** on a PR that has been open for 4+ days with zero commits. The two critical issues (inverted boundary-condition semantics and the reference doc default value contradiction) will cause real developer confusion and potential bugs. The broken link has been flagged in every review. Please address all required changes and push an updated commit. --- **Automated by CleverAgents Bot** Supervisor: PR Review Pool | Agent: pr-self-reviewer
freemo closed this pull request 2026-04-15 15:44:36 +00:00
Some checks are pending
ci.yml / docs: document v3.8.1 changes — skeleton context inheritance, actor YAML-first path, checkpoint triggers (push) Failing after 0s
Details
ci.yml / docs: document v3.8.1 changes — skeleton context inheritance, actor YAML-first path, checkpoint triggers (pull_request) Failing after 0s
Details
CI / build*
Required
CI / coverage*
Required
CI / docker*
Required
CI / integration_tests*
Required
CI / lint*
Required
CI / quality*
Required
CI / security*
Required
CI / typecheck*
Required
CI / unit_tests*
Required

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
HAL9000
Labels
Clear labels   
auto/needs-reevaluation
Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.

  
controller-managed
Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.

  
auto/blocked-by-deps
PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.

  
auto/ci-timeout
Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.

  
auto/claimed-implementer
Currently being processed by an implementer worker.

  
auto/claimed-merge
Currently being processed by the merge driver.

  
auto/claimed-reviewer
Currently being processed by a reviewer worker.

  
auto/driver-down
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.

  
auto/invariant-violation
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.

  
auto/last-attempt-tier-0
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.

  
auto/last-attempt-tier-1
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.

  
auto/last-attempt-tier-2
In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.

  
auto/last-attempt-tier-min
In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.

  
Automation Tracking
Tracking issues used by the AI Automation system for agents to communicate and report.

  
auto/needs-conflict-resolution
Rebase conflict needs LLM conflict-resolver.

  
auto/needs-implementer
Failing CI needs implementer attention.

  
auto/postmortem
Documenting a driver incident or rollback.

  
auto/ready-to-merge
Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.

  
auto/restart-throttled
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.

  
auto/revert
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.

  
auto/sentinel
Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.

  
auto/stale-inactivity
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.

  
auto/unstable
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.

  
Blocked
A ticket in a blocked state and unable to complete until some other task is completed first.

  
Bounty
$100
 A bounty of $100 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$1000
 A bounty of $1000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$10000
 A bounty of $10000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$20
 A bounty of $20 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$2000
 A bounty of $2000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$250
 A bounty of $250 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$50
 A bounty of $50 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$500
 A bounty of $500 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$5000
 A bounty of $5000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$750
 A bounty of $750 for any open-source contributor who provides a MR that solves this issue

  
MoSCoW
Could have
 Could have feature in order to satisfy the epic/legendary.

  
MoSCoW
Must have
 Must have feature in order to satisfy the epic/legendary.

  
MoSCoW
Should have
 Should have feature in order to satisfy the epic/legendary.

  
Needs Feedback
There are questions in the ticket that can not be completed until the project owner provides clarity.

  
Points
1
 1 man-hours worth of work for an expert with no learning curve.

  
Points
13
 13 man-hours worth of work for an expert with no learning curve.

  
Points
2
 2 man-hours worth of work for an expert with no learning curve.

  
Points
21
 21 man-hours worth of work for an expert with no learning curve.

  
Points
3
 3 man-hours worth of work for an expert with no learning curve.

  
Points
34
 34 man-hours worth of work for an expert with no learning curve.

  
Points
5
 5 man-hours worth of work for an expert with no learning curve.

  
Points
55
 55 man-hours worth of work for an expert with no learning curve.

  
Points
8
 8 man-hours worth of work for an expert with no learning curve.

  
Points
88
 88 man-hours worth of work for an expert with no learning curve.

  
Priority
Backlog
 This ticket has backlogged priority and is not to be worked on yet

  
Priority
CI Blocker
 Critical priority issue that blocks CI/CD pipeline and prevents PR merges

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Signed-off: Owner
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Scrum Master
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Tech Lead
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Spike
A ticket for learning a tool or technology that is needed to be able to do future planning and design.

  
State
Completed
 The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.

  
State
Duplicate
 A ticket that represents the same content as an existing ticket.

  
State
In Progress
 A ticket that is actively being developed.

  
State
In Review
 A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.

  
State
Paused
 This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.

  
State
Unverified
 All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.

  
State
Verified
 The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.

  
State
Wont Do
 This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.

  
Type
Automation
 Any edits or discussion about the AI automated coding system.

  
Type
Bug
 Something that doesnt work as intended.

  
Type
Discussion
 Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.

  
Type
Documentation
 An error or improvement needed in the documentation.

  
Type
Epic
 Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.

  
Type
Feature
 Some new functionality not present.

  
Type
Legendary
 A type of Epic which will contain other Epics.

  
Type
Refactor
 A code change that restructures existing code without changing its external behavior.

  
Type
Support
 Someone needs help using the project.

  
Type
Task
 A generic task that doesnt fit into the other type categories.

  
Type
Testing
 Work exclusively focusing on fixing or expanding testing.

No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
Milestone
Clear milestone
No items
No milestone
v3.4.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
HAL9000
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!3696
No description provided.