docs: guard all-pages loop, remove -o json tombstones from cfl test docs - #467
Merged
Conversation
Two leftover findings from #455 review that landed in approving rounds: - chaos-testing.md: -o json page-view row said PASS/Valid JSON, but resource JSON was removed in #392 and now errors at the root. - SearchIssues.md: the collect-all-pages loop did not check the jtk issues list exit status, so a failed page silently truncated results. Guard the command substitution and abort on failure.
… surface Resource JSON output was removed in #392, but the cfl test docs still carried struck-through 'obsolete' rows and checklist items walking through it, plus a chaos-session log that recorded its behavior. Delete those entirely rather than documenting a surface that no longer exists; keep only the live contract statements (OUTPUT_SPEC, ARTIFACT_CONTRACT, changelog, README breaking-change note). Also drop a stale -o json reference from an attachment list test comment.
![rianjs-bot[bot]](https://avatars.githubusercontent.com/u/7378790?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 247413b1986c
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| documentation:docs | 0 |
| policies:conventions | 0 |
| go:implementation-tests | 0 |
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| documentation:docs | complete_broad | skills/Jira/Workflows/SearchIssues.md, tools/cfl/chaos-testing.md, tools/cfl/integration-tests.md, tools/cfl/test-plan-writes.md | unavailable | Documentation-only review; live Confluence integration commands were not run. |
| policies:conventions | complete_broad | tools/cfl/chaos-testing.md, tools/cfl/integration-tests.md, tools/cfl/test-plan-writes.md | unavailable | Shared cli-common and organization automation sources were not available locally; review used the repo-local cfl development guide and output/artifact contracts. |
| go:implementation-tests | complete_broad | tools/cfl/internal/cmd/attachment/list_test.go | unavailable | Review limited to the assigned Go test-file comment-only change. |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 14s | unavailable | gpt-5.6-terra | cr 0.10.260
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | documentation:docs, policies:conventions, go:implementation-tests |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 1m 14s wall · 1m 49s compute |
| Cost | unavailable |
| Tokens | 390.3k in / 5.2k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | gpt-5.6-terra | 15.5k | 488 | 10.5k | unavailable | unavailable | 12s |
| documentation:docs | gpt-5.6-terra | 183.3k | 2.3k | 131.3k | unavailable | unavailable | 49s |
| policies:conventions | gpt-5.6-terra | 120.8k | 1.4k | 86.5k | unavailable | unavailable | 32s |
| go:implementation-tests | gpt-5.6-terra | 37.8k | 503 | 20.0k | unavailable | unavailable | 12s |
| orchestrator-rollup | gpt-5.6-terra | 32.8k | 527 | 25.6k | unavailable | unavailable | 3s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cleans up leftover documentation debt surfaced during the #455 review, plus removes tombstone documentation for the resource JSON surface that was deleted in #392.
Correctness fixes (the original two review stragglers):
skills/Jira/Workflows/SearchIssues.md— the collect-all-pages loop did not check thejtk issues listexit status, so a failed page request silently printed an empty value and terminated, returning a partial result set. The command substitution is now guarded and aborts on failure.Tombstone removal —
-o json/-o yamlresource output (removed in #392):The cfl test docs still carried struck-through "obsolete" rows, checklist items, and a chaos-session log walking through the removed resource JSON/YAML output. Documenting a surface that no longer exists is pure noise, so these are deleted outright:
tools/cfl/integration-tests.md— removed all~~JSON output~~ (#392 removed)table rows and checklist items (both auth-method runs), the top-of-file cfl: remove resource-read -o json #392 note, and the JSON row from the Output Formats table.tools/cfl/test-plan-writes.md— removed the obsolete JSON test rows and the cfl: remove resource-read -o json #392 note.tools/cfl/chaos-testing.md— removed the historical-o json/-o yamlbug rows, test-progress rows, and note; corrected the summary bug counts accordingly.tools/cfl/internal/cmd/attachment/list_test.go— reworded a test comment that referenced the removed-o jsonto state the current behavior directly.Kept intentionally: the live contract statements that specify current behavior (OUTPUT_SPEC's "resource
-o jsonmust fail at the root", ARTIFACT_CONTRACT's valid-format set, the changelog, and the README breaking-change note). Those document what the tool does now (reject it), not a dead feature.Docs-and-comments only.
make check(build, test, lint) is green.