test(e2e): TDD behavioral test proving ACMS indexing pipeline is not wired into CLI (bug #1028) #1124
No reviewers
Labels
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
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Blocks
#1029 TDD: ACMS indexing pipeline not wired into CLI — ContextTierService starts empty (bug #1028)
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core!1124
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "tdd/m5-acms-cli-indexing-pipeline-wiring"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
This PR adds a Robot Framework E2E test suite (
robot/e2e/tdd_acms_behavioral_validation.robot) that proves bug #1028 exists — the ACMS indexing pipeline is not wired into the CLI, soContextTierServicestarts empty on every invocation.Changes
robot/e2e/tdd_acms_behavioral_validation.robot— 4 E2E test cases taggedtdd_expected_fail,tdd_bug,tdd_bug_1028,E2Erobot/e2e/common_e2e.resource— Extracted shared keywords (Run CLI,Extract JSON From Stdout,Link Resource To Project,Create Synthetic Codebase) from bothm5_acceptance.robotandtdd_acms_behavioral_validation.robotto eliminate ~97 lines of duplication.Create Synthetic Codebaseis parameterized withproject_label.robot/e2e/m5_acceptance.robot— Removed duplicated keywords now provided bycommon_e2e.resource.## Unreleaseddocumenting the TDD tests for #1029.Test Cases
fragment_count > 0(fails, proving bug)fragment_count > 0withmax_file_sizepolicy (fails, proving bug). Includes TODO comment for post-fix exclusion assertion.fragment_count > 0(fails, proving bug). Includes explicit developer responsibility note about git-tracking of generated files.All tests pass CI through result inversion by the
tdd_expected_fail_listener.py— failing assertions (bug confirmed) are inverted to PASS.Documentation & Robustness Improvements
tdd_expected_failresult inversion scope.(rc=${var.rc}). Check DEBUG logs above.for debugging consistency withm5_acceptance.robot.Run CLIcalls removed (Run CLI already validates rc internally).Run CLIkeyword documentation includes API key security notes.TODO(bugfix/...)comment for the bug-fix developer.Review Fix Round
Addressed all findings from Luis's review (review #2691):
CorrectionServicechangelog entry (50 lines) accidentally deleted during merge conflict resolution.common_e2e.resource(parameterizedCreate Synthetic Codebase, sharedRun CLI,Extract JSON From Stdout,Link Resource To Project).(rc=...). Check DEBUG logs above.Run CLIcalls.tdd_expected_failmasking is documented and mitigated.Motivation
Per the Bug Fix Workflow in CONTRIBUTING.md, this TDD issue (#1029) is the prerequisite for bug fix #1028. The tests capture the buggy behavior so that when the fix is implemented, removing the
tdd_expected_failtag will cause the tests to pass normally.Quality Gates
Closes #1029
39bfe4c3a1a2f91cdb27a2f91cdb27640e90a2a5Code Review Report — PR #1124 (TDD Bug #1028 / Issue #1029)
Branch:
tdd/m5-acms-cli-indexing-pipeline-wiringCommit:
640e90aby Rui HuReview scope: Branch diff vs
origin/master(2 files changed:CHANGELOG.md,robot/e2e/tdd_acms_behavioral_validation.robot)Methodology: 3 iterative full-spectrum review cycles covering bug detection, test flaws, test coverage, performance, security, spec compliance, and code quality. Findings stabilised after cycle 2.
Positive Observations
tdd_expected_fail,tdd_bug,tdd_bug_1028,E2E) followsCONTRIBUTING.md > TDD Bug Test Tagsrules and will pass the listener's_validate_tdd_tags()checks.tdd_expected_fail_listener.pyIS registered in thee2e_testsnox session (line 723 ofnoxfile.py), so result inversion will work correctly.project context simulate,project context inspect,project context setwith--include-path,--max-file-size,--max-total-size) all match the specification command synopsis (lines 237–271 ofdocs/specification.md).tdd_expected_failmasking setup failures is explicitly documented in the suite header and mitigated bym5_acceptance.robot.NO_COLOR=1is set, stdout/stderr are not embedded in assertion messages, and API key logging risk is mitigated by DEBUG-level logging.ISSUES CLOSED: #1029is correct per the TDD workflow.Findings by Severity
CRITICAL — Bug Detection
C1. CHANGELOG regression: #845 CorrectionService entry accidentally deleted
CHANGELOG.md, lines 72–121 of merge-base content## Unreleasedsection — the entire#845CorrectionServicesubtree isolation entry. This entry documents numerous fixes (BFS cycle detection, dry-run enforcement, terminal-state guards, mode validation, status restoration, etc.) from PR #845 merged on 2026-03-19.git diff origin/master...HEAD -- CHANGELOG.mdshows the merge-base CHANGELOG has 946 lines; the branch HEAD has 900 lines (946 − 50 deleted + 4 added = 900).#1029entry at the top of the Unreleased section. The#845entry, which was adjacent to the conflict region, was dropped during resolution.#845changelog documentation will be permanently lost from release notes.#845entry fromorigin/master'sCHANGELOG.mdbefore merging.MEDIUM — Test Flaws
M1. Large Project test: 10K generated files are not committed to the git repo
robot/e2e/tdd_acms_behavioral_validation.robot, lines 317–338.pyfiles on the filesystem but does notgit add+git committhem. The workspace resource is registered asgit-checkouttype (line 83). If the bug #1028 fix routes indexing through the git sandbox (which only exposes git-tracked content), these files will not be visible and the test will fail for the wrong reason (files not tracked vs. pipeline not wired).walk_and_indexinrepo_indexing_utils.pyusesos.walk()(filesystem), notgit ls-files, so this may not manifest.git add/commitcommands ready to uncomment.m5_acceptance.robothas the same pattern (neither suite commits generated files).git add/commitlines now (or adding a comment making it clear this is a deliberate risk acceptance for the TDD phase, and the bug-fix developer must evaluate it). Current phrasing ("If the ACMS indexing pipeline reads only git-tracked content...") is ambiguous about responsibility.M2. Budget enforcement test is a partial assertion
tdd_acms_behavioral_validation.robot, lines 260–301fragment_count > 0. It does not verify thatlarge_file.py(>1 KiB) is excluded from the fragment list while smaller files are included. The test title claims it tests exclusion, but the assertion only tests inclusion.large_file.pyexclusion check.LOW — Code Quality
L1. ~97 lines of keyword/setup code duplicated from
m5_acceptance.robottdd_acms_behavioral_validation.robot, keyword section (lines 46–167)m5_acceptance.robot:Run CLI(18 lines),Extract JSON From Stdout(15 lines),Link Resource To Project(6 lines),Suite Teardown(3 lines).Create Synthetic CodebaseandSuite Setupdiffer only in trivial string literals (~55 lines). Total: ~97 duplicated lines out of ~105 keyword-definition lines.common_e2e.resource(which already exists and is imported by both files). The keywords can be parameterized with suite-specific names (resource name, workspace label) passed as arguments. This would reduce maintenance burden when CLI interfaces change.L2. Redundant exit code assertions
tdd_acms_behavioral_validation.robot, lines 201, 238, 275, 341Should Be Equal As Integers ${result.rc} 0is called immediately afterRun CLIwhich already validates the exit code internally viaShould Be Equal As Integers ${result.rc} ${expected_rc}(default 0). These assertions are always true ifRun CLIdidn't fail.L3. Suite setup error messages less verbose than
m5_acceptance.robottdd_acms_behavioral_validation.robot, lines 66, 71, 73msg=git init failedvs.m5_acceptance.robot'smsg=git init failed (rc=${git_init.rc}). Check DEBUG logs above.The less-verbose messages omit the actual return code, making debugging harder if git commands fail in CI.(rc=${variable.rc})and "Check DEBUG logs" to match them5_acceptance.robotpattern for consistency.INFORMATIONAL — Known Limitations (documented, no action required)
I1.
tdd_expected_faillistener masks unrelated failuresm5_acceptance.robotrunning the same CLI plumbing withouttdd_expected_failinversion.Verdict
Request changes — the CHANGELOG regression (C1) must be fixed before merge. The #845 entry needs to be restored from
origin/master. All other findings are medium/low severity and can be addressed at the reviewer's discretion.Review performed using 3 iterative full-spectrum cycles (bug detection, test flaws, test coverage, performance, security, spec compliance, code quality). Findings stabilised after cycle 2 with no new issues found in cycle 3.
@ -3,2 +3,4 @@## Unreleased- Added TDD bug-capture E2E tests for bug #1028 — ACMS indexing pipeline notwired into CLI. Four Robot Framework E2E tests prove ContextTierService startsCRITICAL (C1): This diff removes the entire #845 CorrectionService changelog entry (50 lines). The merge-base (
9e316b1) contains this entry at lines 72-121, but it is absent from the branch HEAD. This is likely a merge conflict resolution error — when adding the 4-line #1029 entry, the adjacent #845 block was accidentally dropped.Action required: Restore the #845 entry from
origin/masterbefore merging. You can extract it with:and re-insert the block between the
(#331)entry and the deferred physical resource types entry.@ -0,0 +63,4 @@Create Synthetic Codebase ${ws}# Initialize a git repository${git_init}= Run Process git init cwd=${ws} timeout=60s on_timeout=killShould Be Equal As Integers ${git_init.rc} 0 msg=git init failedLOW (L3): Error message is
msg=git init failed— less verbose thanm5_acceptance.robot's equivalentmsg=git init failed (rc=${git_init.rc}). Check DEBUG logs above.Consider adding the return code and log hint for debugging consistency.@ -0,0 +116,4 @@${large_content}= Evaluate "# auto-generated large file\\n" + ("x = 1\\n" * 250)Create File ${base_dir}${/}large_file.py ${large_content}Run CLILOW (L1): This
Run CLIkeyword is character-for-character identical to the one inm5_acceptance.robot(lines 109-126). Along withExtract JSON From Stdout,Link Resource To Project,Create Synthetic Codebase, and the Suite Setup/Teardown, approximately 97 lines of keyword code are duplicated between the two files. Consider extracting the common keywords intocommon_e2e.resource(already imported by both files) with parameterized suite-specific names.@ -0,0 +257,4 @@Should Be True ${total_indexed} > 0... msg=Bug #1028: total indexed fragments is ${total_indexed} (expected > 0). tier_metrics: hot=${hot_count}, warm=${warm_count}, cold=${cold_count}. ACMS indexing pipeline is not wired into CLI.Budget Enforcement Excludes Oversized FilesMEDIUM (M2): This test title says "Excludes Oversized Files" but the assertion only checks
fragment_count > 0(that some files are indexed). It does not verify thatlarge_file.pyis actually excluded. The TODO on lines 295-298 documents this gap — ensure the bug-fix developer adds the exclusion assertion when wiring the pipeline.@ -0,0 +328,4 @@${gen}= Run Process ${PYTHON} ${gen_script} ${scale_dir}... timeout=120s on_timeout=killShould Be Equal As Integers ${gen.rc} 0 msg=10K file generation failed# NOTE: The 10K generated files are written to the filesystem but NOTMEDIUM (M1): The 10K generated files are written to the filesystem but NOT committed to the workspace git repo. The resource is registered as
git-checkouttype (line 83). If the bug-fix routes indexing through the git sandbox, these untracked files won't be visible and the test will fail for the wrong reason.The NOTE comment here is good and documents the risk, but consider making the responsibility assignment more explicit — e.g.: "The bug-fix developer MUST evaluate whether the fix uses filesystem or git-tracked content and uncomment these lines if needed."
640e90a2a55602c9a87cResponse to Review #2691 (Luis)
Thanks for the thorough review! All findings addressed in commit
5602c9a8. Summary:C1 (CRITICAL) — CHANGELOG #845 entry deleted ✅ Fixed
Reset
CHANGELOG.mdtoorigin/masterand re-added the#1029entry. The #845CorrectionServiceentry (50 lines) is now fully restored.M1 (MEDIUM) — 10K files not committed to git ✅ Fixed
Replaced the ambiguous NOTE with an explicit responsibility assignment: "The bug-fix developer MUST evaluate whether the fix routes indexing through the git sandbox (which only exposes git-tracked content) or the filesystem."
M2 (MEDIUM) — Budget enforcement partial assertion ℹ️ Acknowledged
Agreed — the TODO comment already documents the gap, and the assertion is appropriate for the TDD capture phase. The bug-fix developer will add the
large_file.pyexclusion assertion when wiring the pipeline.L1 (LOW) — ~97 lines duplicated from m5_acceptance.robot ✅ Fixed
Extracted
Run CLI,Extract JSON From Stdout,Link Resource To Project, andCreate Synthetic Codebaseintocommon_e2e.resource.Create Synthetic Codebaseis parameterized with${project_label}for suite-specific literals. Bothm5_acceptance.robotandtdd_acms_behavioral_validation.robotnow use the shared keywords.L2 (LOW) — Redundant exit code assertions ✅ Fixed
Removed all 12 instances of redundant
Should Be Equal As Integers ${result.rc} 0that followedRun CLIcalls.L3 (LOW) — Suite setup error messages less verbose ✅ Fixed
All git command error messages now include
(rc=${var.rc}). Check DEBUG logs above.matching them5_acceptance.robotpattern.I1 (INFO) —
tdd_expected_faillistener masks ℹ️ AcknowledgedAlready documented. No action needed.
Review: APPROVED
TDD E2E behavioral test proving ACMS indexing pipeline is not wired into CLI (bug #1028). Clean test structure with proper tags and clear documentation of what the test proves.
5602c9a87c90d18f06d1New commits pushed, approval review dismissed automatically according to repository settings